The cpu model is the single device available in user-mode.
Since we want to restrict some fields to user-mode emulation,
we prefer to set the vmsd field of CPUClass, rather than the
DeviceClass one.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/alpha/cpu.c      | 2 +-
 target/cris/cpu.c       | 2 +-
 target/hppa/cpu.c       | 2 +-
 target/m68k/cpu.c       | 2 +-
 target/microblaze/cpu.c | 2 +-
 target/openrisc/cpu.c   | 2 +-
 target/sh4/cpu.c        | 2 +-
 target/unicore32/cpu.c  | 2 +-
 target/xtensa/cpu.c     | 2 +-
 9 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index 27192b62e22..faabffe0796 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -237,7 +237,7 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_write_register = alpha_cpu_gdb_write_register;
 #ifndef CONFIG_USER_ONLY
     cc->get_phys_page_debug = alpha_cpu_get_phys_page_debug;
-    dc->vmsd = &vmstate_alpha_cpu;
+    cc->vmsd = &vmstate_alpha_cpu;
 #endif
     cc->disas_set_info = alpha_cpu_disas_set_info;
 
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index ed983380fca..29a865b75d2 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -293,7 +293,7 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_write_register = cris_cpu_gdb_write_register;
 #ifndef CONFIG_USER_ONLY
     cc->get_phys_page_debug = cris_cpu_get_phys_page_debug;
-    dc->vmsd = &vmstate_cris_cpu;
+    cc->vmsd = &vmstate_cris_cpu;
 #endif
 
     cc->gdb_num_core_regs = 49;
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index d8fad52d1fe..4f142de6e45 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -162,7 +162,7 @@ static void hppa_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_write_register = hppa_cpu_gdb_write_register;
 #ifndef CONFIG_USER_ONLY
     cc->get_phys_page_debug = hppa_cpu_get_phys_page_debug;
-    dc->vmsd = &vmstate_hppa_cpu;
+    cc->vmsd = &vmstate_hppa_cpu;
 #endif
     cc->disas_set_info = hppa_cpu_disas_set_info;
     cc->gdb_num_core_regs = 128;
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 37d2ed9dc79..c98fb1e33be 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -533,7 +533,7 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
     cc->gdb_write_register = m68k_cpu_gdb_write_register;
 #if defined(CONFIG_SOFTMMU)
     cc->get_phys_page_debug = m68k_cpu_get_phys_page_debug;
-    dc->vmsd = &vmstate_m68k_cpu;
+    cc->vmsd = &vmstate_m68k_cpu;
 #endif
     cc->disas_set_info = m68k_cpu_disas_set_info;
 
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 433ba202037..335dfdc734e 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -387,7 +387,7 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
 
 #ifndef CONFIG_USER_ONLY
     cc->get_phys_page_attrs_debug = mb_cpu_get_phys_page_attrs_debug;
-    dc->vmsd = &vmstate_mb_cpu;
+    cc->vmsd = &vmstate_mb_cpu;
 #endif
     device_class_set_props(dc, mb_properties);
     cc->gdb_num_core_regs = 32 + 27;
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index 2c64842f46b..79d246d1930 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -204,7 +204,7 @@ static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_write_register = openrisc_cpu_gdb_write_register;
 #ifndef CONFIG_USER_ONLY
     cc->get_phys_page_debug = openrisc_cpu_get_phys_page_debug;
-    dc->vmsd = &vmstate_openrisc_cpu;
+    cc->vmsd = &vmstate_openrisc_cpu;
 #endif
     cc->gdb_num_core_regs = 32 + 3;
     cc->disas_set_info = openrisc_disas_set_info;
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index ac65c88f1f8..bd44de53729 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -262,7 +262,7 @@ static void superh_cpu_class_init(ObjectClass *oc, void *data)
 
     cc->gdb_num_core_regs = 59;
 
-    dc->vmsd = &vmstate_sh_cpu;
+    cc->vmsd = &vmstate_sh_cpu;
     cc->tcg_ops = &superh_tcg_ops;
 }
 
diff --git a/target/unicore32/cpu.c b/target/unicore32/cpu.c
index 0258884f845..12894ffac6a 100644
--- a/target/unicore32/cpu.c
+++ b/target/unicore32/cpu.c
@@ -146,7 +146,7 @@ static void uc32_cpu_class_init(ObjectClass *oc, void *data)
     cc->dump_state = uc32_cpu_dump_state;
     cc->set_pc = uc32_cpu_set_pc;
     cc->get_phys_page_debug = uc32_cpu_get_phys_page_debug;
-    dc->vmsd = &vmstate_uc32_cpu;
+    cc->vmsd = &vmstate_uc32_cpu;
     cc->tcg_ops = &uc32_tcg_ops;
 }
 
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index e2b2c7a71c1..6bedd5b97b8 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -218,7 +218,7 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
     cc->get_phys_page_debug = xtensa_cpu_get_phys_page_debug;
 #endif
     cc->disas_set_info = xtensa_cpu_disas_set_info;
-    dc->vmsd = &vmstate_xtensa_cpu;
+    cc->vmsd = &vmstate_xtensa_cpu;
     cc->tcg_ops = &xtensa_tcg_ops;
 }
 
-- 
2.26.2

On Tue, Mar 02, 2021 at 03:57:52PM +0100, Philippe Mathieu-Daudé wrote:
> The cpu model is the single device available in user-mode.
> Since we want to restrict some fields to user-mode emulation,
> we prefer to set the vmsd field of CPUClass, rather than the
> DeviceClass one.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Is this going to have an externally visible effect?

If it does, how can we make sure it's safe?

If it does not, do you know why CPUClass::vmsd exists in the
first place?

Do you think it would be simpler to just squash this patch into
  [PATCH v3 08/27] cpu: Move CPUClass::vmsd to SysemuCPUOps
?

-- 
Eduardo

+Juan

On 4/22/21 12:03 AM, Eduardo Habkost wrote:
> On Tue, Mar 02, 2021 at 03:57:52PM +0100, Philippe Mathieu-Daudé wrote:
>> The cpu model is the single device available in user-mode.
>> Since we want to restrict some fields to user-mode emulation,
>> we prefer to set the vmsd field of CPUClass, rather than the
>> DeviceClass one.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Is this going to have an externally visible effect?

On system emulation, no, because unchanged.

On user emulation migration it is not used, the symbol is here
to satisfy linking:

include/hw/core/cpu.h-1070-#ifdef CONFIG_SOFTMMU
include/hw/core/cpu.h-1071-extern const VMStateDescription
vmstate_cpu_common;
include/hw/core/cpu.h-1072-#else
include/hw/core/cpu.h:1073:#define vmstate_cpu_common vmstate_dummy
include/hw/core/cpu.h-1074-#endif
include/migration/vmstate.h:197:extern const VMStateDescription
vmstate_dummy;
stubs/vmstate.c:4:const VMStateDescription vmstate_dummy = {};

> If it does, how can we make sure it's safe?
> 
> If it does not, do you know why CPUClass::vmsd exists in the
> first place?

My guess is CPUState is the only device used in user emulation,
so it would be a way to restrict the vmstate_dummy to CPU and
not to any DeviceState?

But looking at the introductory commit:

commit b170fce3dd06372f7bfec9a780ebcb1fce6d57e4
Author: Andreas Färber <afaerber@suse.de>
Date:   Sun Jan 20 20:23:22 2013 +0100

    cpu: Register VMStateDescription through CPUState

    In comparison to DeviceClass::vmsd, CPU VMState is split in two,
    "cpu_common" and "cpu", and uses cpu_index as instance_id instead of -1.
    Therefore add a CPU-specific CPUClass::vmsd field.

    Unlike the legacy CPUArchState registration, rather register CPUState.

Juan, do you remember?

> 
> Do you think it would be simpler to just squash this patch into
>   [PATCH v3 08/27] cpu: Move CPUClass::vmsd to SysemuCPUOps
> ?

Certainly cleaner, I'll respin!

Thanks,

Phil.

On Thu, 22 Apr 2021 at 10:55, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> My guess is CPUState is the only device used in user emulation,
> so it would be a way to restrict the vmstate_dummy to CPU and
> not to any DeviceState?
>
> But looking at the introductory commit:
>
> commit b170fce3dd06372f7bfec9a780ebcb1fce6d57e4
> Author: Andreas Färber <afaerber@suse.de>
> Date:   Sun Jan 20 20:23:22 2013 +0100
>
>     cpu: Register VMStateDescription through CPUState
>
>     In comparison to DeviceClass::vmsd, CPU VMState is split in two,
>     "cpu_common" and "cpu", and uses cpu_index as instance_id instead of -1.
>     Therefore add a CPU-specific CPUClass::vmsd field.
>
>     Unlike the legacy CPUArchState registration, rather register CPUState.
>
> Juan, do you remember?

Oh yes, I remember this. There are two ways to handle migration for
a CPU object:

 (1) like any other device, so it has a dc->vmsd that covers
migration for the whole object. As usual for objects that are a
subclass of a parent that has state, the first entry in the
VMStateDescription field list is VMSTATE_CPU(), which migrates
the cpu_common fields, followed by whatever the CPU's own migration
fields are.
 (2) a backwards-compatible mechanism for CPUs that were
originally migrated using manual "write fields to the migration
stream structures". The on-the-wire migration format
for those is based on the 'env' pointer (which isn't a QOM object),
and the cpu_common part of the migration data is elsewhere.

cpu_exec_realizefn() handles both possibilities:
 * for type 1, dc->vmsd is set and cc->vmsd is not,
   so cpu_exec_realizefn() does nothing, and the standard
   "register dc->vmsd for a device" code does everything needed
 * for type 2, dc->vmsd is NULL and so we register the vmstate_cpu_common
   directly to handle the cpu-common fields, and the cc->vmsd to handle
   the per-CPU stuff

You can't change a CPU from one type to the other without breaking
migration compatibility, which is why some guest architectures
are stuck on the cc->vmsd form. New targets should use dc->vmsd.

thanks
-- PMM

On 4/22/21 12:28 PM, Peter Maydell wrote:
> On Thu, 22 Apr 2021 at 10:55, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> My guess is CPUState is the only device used in user emulation,
>> so it would be a way to restrict the vmstate_dummy to CPU and
>> not to any DeviceState?
>>
>> But looking at the introductory commit:
>>
>> commit b170fce3dd06372f7bfec9a780ebcb1fce6d57e4
>> Author: Andreas Färber <afaerber@suse.de>
>> Date:   Sun Jan 20 20:23:22 2013 +0100
>>
>>     cpu: Register VMStateDescription through CPUState
>>
>>     In comparison to DeviceClass::vmsd, CPU VMState is split in two,
>>     "cpu_common" and "cpu", and uses cpu_index as instance_id instead of -1.
>>     Therefore add a CPU-specific CPUClass::vmsd field.
>>
>>     Unlike the legacy CPUArchState registration, rather register CPUState.
>>
>> Juan, do you remember?
> 
> Oh yes, I remember this. There are two ways to handle migration for
> a CPU object:
> 
>  (1) like any other device, so it has a dc->vmsd that covers
> migration for the whole object. As usual for objects that are a
> subclass of a parent that has state, the first entry in the
> VMStateDescription field list is VMSTATE_CPU(), which migrates
> the cpu_common fields, followed by whatever the CPU's own migration
> fields are.
>  (2) a backwards-compatible mechanism for CPUs that were
> originally migrated using manual "write fields to the migration
> stream structures". The on-the-wire migration format
> for those is based on the 'env' pointer (which isn't a QOM object),
> and the cpu_common part of the migration data is elsewhere.
> 
> cpu_exec_realizefn() handles both possibilities:
>  * for type 1, dc->vmsd is set and cc->vmsd is not,
>    so cpu_exec_realizefn() does nothing, and the standard
>    "register dc->vmsd for a device" code does everything needed
>  * for type 2, dc->vmsd is NULL and so we register the vmstate_cpu_common
>    directly to handle the cpu-common fields, and the cc->vmsd to handle
>    the per-CPU stuff
> 
> You can't change a CPU from one type to the other without breaking
> migration compatibility, which is why some guest architectures
> are stuck on the cc->vmsd form. New targets should use dc->vmsd.

Doh I just post v5. I guess I'll have to revisit patch #7, because
I likely blew type 2 migration:
https://patchew.org/QEMU/20210422104705.2454166-1-f4bug@amsat.org/20210422104705.2454166-8-f4bug@amsat.org/

On 4/22/21 1:01 PM, Philippe Mathieu-Daudé wrote:
> On 4/22/21 12:28 PM, Peter Maydell wrote:
>> On Thu, 22 Apr 2021 at 10:55, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> My guess is CPUState is the only device used in user emulation,
>>> so it would be a way to restrict the vmstate_dummy to CPU and
>>> not to any DeviceState?
>>>
>>> But looking at the introductory commit:
>>>
>>> commit b170fce3dd06372f7bfec9a780ebcb1fce6d57e4
>>> Author: Andreas Färber <afaerber@suse.de>
>>> Date:   Sun Jan 20 20:23:22 2013 +0100
>>>
>>>     cpu: Register VMStateDescription through CPUState
>>>
>>>     In comparison to DeviceClass::vmsd, CPU VMState is split in two,
>>>     "cpu_common" and "cpu", and uses cpu_index as instance_id instead of -1.
>>>     Therefore add a CPU-specific CPUClass::vmsd field.
>>>
>>>     Unlike the legacy CPUArchState registration, rather register CPUState.
>>>
>>> Juan, do you remember?
>>
>> Oh yes, I remember this. There are two ways to handle migration for
>> a CPU object:
>>
>>  (1) like any other device, so it has a dc->vmsd that covers
>> migration for the whole object. As usual for objects that are a
>> subclass of a parent that has state, the first entry in the
>> VMStateDescription field list is VMSTATE_CPU(), which migrates
>> the cpu_common fields, followed by whatever the CPU's own migration
>> fields are.

target/alpha/cpu.c:240:    dc->vmsd = &vmstate_alpha_cpu;
target/cris/cpu.c:296:    dc->vmsd = &vmstate_cris_cpu;
target/hppa/cpu.c:165:    dc->vmsd = &vmstate_hppa_cpu;
target/m68k/cpu.c:537:    dc->vmsd = &vmstate_m68k_cpu;
target/microblaze/cpu.c:390:    dc->vmsd = &vmstate_mb_cpu;
target/openrisc/cpu.c:207:    dc->vmsd = &vmstate_openrisc_cpu;
target/sh4/cpu.c:265:    dc->vmsd = &vmstate_sh_cpu;
target/unicore32/cpu.c:149:    dc->vmsd = &vmstate_uc32_cpu;
target/xtensa/cpu.c:221:    dc->vmsd = &vmstate_xtensa_cpu;

>>  (2) a backwards-compatible mechanism for CPUs that were
>> originally migrated using manual "write fields to the migration
>> stream structures". The on-the-wire migration format
>> for those is based on the 'env' pointer (which isn't a QOM object),
>> and the cpu_common part of the migration data is elsewhere.

target/arm/cpu.c:1985:    cc->vmsd = &vmstate_arm_cpu;
target/avr/cpu.c:216:    cc->vmsd = &vms_avr_cpu;
target/i386/cpu.c:7434:    cc->vmsd = &vmstate_x86_cpu;
target/lm32/cpu.c:244:    cc->vmsd = &vmstate_lm32_cpu;
target/mips/cpu.c:723:    cc->vmsd = &vmstate_mips_cpu;
target/moxie/cpu.c:125:    cc->vmsd = &vmstate_moxie_cpu;
target/ppc/translate_init.c.inc:10923:    cc->vmsd = &vmstate_ppc_cpu;
target/riscv/cpu.c:627:    cc->vmsd = &vmstate_riscv_cpu;
target/s390x/cpu.c:520:    cc->vmsd = &vmstate_s390_cpu;
target/sparc/cpu.c:892:    cc->vmsd = &vmstate_sparc_cpu;

>> cpu_exec_realizefn() handles both possibilities:
>>  * for type 1, dc->vmsd is set and cc->vmsd is not,
>>    so cpu_exec_realizefn() does nothing, and the standard
>>    "register dc->vmsd for a device" code does everything needed
>>  * for type 2, dc->vmsd is NULL and so we register the vmstate_cpu_common
>>    directly to handle the cpu-common fields, and the cc->vmsd to handle
>>    the per-CPU stuff
>>
>> You can't change a CPU from one type to the other without breaking
>> migration compatibility, which is why some guest architectures
>> are stuck on the cc->vmsd form. New targets should use dc->vmsd.

IOW new targets should use type 1.


Looking at type 2:

a) targets added 'recently' with the incorrect type 2:

target/avr/cpu.c:216:    cc->vmsd = &vms_avr_cpu;
target/riscv/cpu.c:627:    cc->vmsd = &vmstate_riscv_cpu;

b) targets where migration isn't really an issue:

target/lm32/cpu.c:244:    cc->vmsd = &vmstate_lm32_cpu;
target/moxie/cpu.c:125:    cc->vmsd = &vmstate_moxie_cpu;

c) targets where migration could be broken:

target/mips/cpu.c:723:    cc->vmsd = &vmstate_mips_cpu;
target/sparc/cpu.c:892:    cc->vmsd = &vmstate_sparc_cpu;

d) KVM targets ("security boundary" or Tier-1) are left:

target/arm/cpu.c:1985:    cc->vmsd = &vmstate_arm_cpu;
target/i386/cpu.c:7434:    cc->vmsd = &vmstate_x86_cpu;
target/ppc/translate_init.c.inc:10923:    cc->vmsd = &vmstate_ppc_cpu;
target/s390x/cpu.c:520:    cc->vmsd = &vmstate_s390_cpu;


Isn't "machine type" what allows us to change migration stream?
All targets in d) do support that.


How could we enforce no new type 2 targets?


Thanks,

Phil.

On Thu, 22 Apr 2021 at 16:41, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> IOW new targets should use type 1.
>
>
> Looking at type 2:
>
> a) targets added 'recently' with the incorrect type 2:
>
> target/avr/cpu.c:216:    cc->vmsd = &vms_avr_cpu;
> target/riscv/cpu.c:627:    cc->vmsd = &vmstate_riscv_cpu;
>
> b) targets where migration isn't really an issue:
>
> target/lm32/cpu.c:244:    cc->vmsd = &vmstate_lm32_cpu;
> target/moxie/cpu.c:125:    cc->vmsd = &vmstate_moxie_cpu;
>
> c) targets where migration could be broken:
>
> target/mips/cpu.c:723:    cc->vmsd = &vmstate_mips_cpu;
> target/sparc/cpu.c:892:    cc->vmsd = &vmstate_sparc_cpu;
>
> d) KVM targets ("security boundary" or Tier-1) are left:
>
> target/arm/cpu.c:1985:    cc->vmsd = &vmstate_arm_cpu;
> target/i386/cpu.c:7434:    cc->vmsd = &vmstate_x86_cpu;
> target/ppc/translate_init.c.inc:10923:    cc->vmsd = &vmstate_ppc_cpu;
> target/s390x/cpu.c:520:    cc->vmsd = &vmstate_s390_cpu;
>
>
> Isn't "machine type" what allows us to change migration stream?
> All targets in d) do support that.

Versioned machine types allow us to change the migration stream
if we do it in a forwards-compatible way (and if we're feeling
kind to RH as a downstream perhaps even backwards-compatible);
but new QEMU still has to be able to read the migration stream
produced by old QEMU for the "virt-5.2" board, or whatever.

In some cases I think we also like to maintain migration
compat on a "best-effort" basis; I think Mark likes to handle
the SPARC guests that way.

thanks
-- PMM

On 4/22/21 5:53 PM, Peter Maydell wrote:
> On Thu, 22 Apr 2021 at 16:41, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> IOW new targets should use type 1.
>>
>>
>> Looking at type 2:
>>
>> a) targets added 'recently' with the incorrect type 2:
>>
>> target/avr/cpu.c:216:    cc->vmsd = &vms_avr_cpu;
>> target/riscv/cpu.c:627:    cc->vmsd = &vmstate_riscv_cpu;
>>
>> b) targets where migration isn't really an issue:
>>
>> target/lm32/cpu.c:244:    cc->vmsd = &vmstate_lm32_cpu;
>> target/moxie/cpu.c:125:    cc->vmsd = &vmstate_moxie_cpu;
>>
>> c) targets where migration could be broken:
>>
>> target/mips/cpu.c:723:    cc->vmsd = &vmstate_mips_cpu;
>> target/sparc/cpu.c:892:    cc->vmsd = &vmstate_sparc_cpu;
>>
>> d) KVM targets ("security boundary" or Tier-1) are left:
>>
>> target/arm/cpu.c:1985:    cc->vmsd = &vmstate_arm_cpu;
>> target/i386/cpu.c:7434:    cc->vmsd = &vmstate_x86_cpu;
>> target/ppc/translate_init.c.inc:10923:    cc->vmsd = &vmstate_ppc_cpu;
>> target/s390x/cpu.c:520:    cc->vmsd = &vmstate_s390_cpu;
>>
>>
>> Isn't "machine type" what allows us to change migration stream?
>> All targets in d) do support that.
> 
> Versioned machine types allow us to change the migration stream
> if we do it in a forwards-compatible way (and if we're feeling
> kind to RH as a downstream perhaps even backwards-compatible);
> but new QEMU still has to be able to read the migration stream
> produced by old QEMU for the "virt-5.2" board, or whatever.
> 
> In some cases I think we also like to maintain migration
> compat on a "best-effort" basis; I think Mark likes to handle
> the SPARC guests that way.

What I don't understand if is there is a possibility to eventually
remove the CPUClass::vmsd (even long and painful), or if it is
directly impossible and we are doomed to keep maintaining both.