Re: [PATCH v28 00/23] i386 cleanup PART 2

[Paolo Bonzini <pbonzini@redhat.com>]

Looks good to me, thanks for all the effort!

Paolo

On 22/03/21 14:27, Claudio Fontana wrote:
> v27 -> v28:
> 
> * rebased on latest master;
>    I indicated the conflicts for the affected patches in the commit message,
>    in case a new review/eye is required.
> 
> * added three patches:
>    - accel: move call to accel_init_interfaces
> 
>      This matches more closely the initialization phases definitions (Paolo)
> 
>    - accel: add init_accel_cpu for adapting accel behavior to cpu type
>    
>      This in particular is useful for ARM, that needs different TCG behavior
>      depending on the CPU subclass.
> 
>    - XXX RFC accel: add cpu_reset
> 
>      This adds an accel cpu behavior to execute after CPU reset.
>      This can be used on x86, arm, s390x, mips for KVM and TCG.
> 
>      The RFC nature of this has to do with the fact that cpu_reset() remains
>      in hw/core/cpu.c , which is common_ss,
>      and cpu_reset() calls accel_cpu_reset() which is in specific_ss.
>      
>      So it seems weird that this builds fine, and all tests seem to pass,
>      without using a specific_ss call.
> 
> 
> v26 -> v27: rebased on latest master
> 
> 
> v25 -> v26:
> * i386: separate fpu_helper into user and sysemu parts
>   - removed the splitting of the user mode into their own user/fpu_helper.c,
>     seems not worth it.
> 
> v24 -> v25:
> 
> * i386: separate fpu_helper into user and sysemu parts
> 
>   - added 2 preliminary patches to the series (from Richard)
>   - rebased on those
> 
> * i386: move TCG btp_helper into sysemu/
> 
>   - fixed title typo (Alex)
>   - nested #ifdef more easily (Richard)
> 
> v23 -> v24:
> 
> * i386: gdbstub: only write CR0/CR2/CR3/EFER for sysemu
>   - remove additional #ifdef TARGET_X86_64
>   - split in two patches, so it is easier to understand.
> 
> v22 -> v23:
> 
> * i386: move TCG btp_helper into sysemu/
>   - extended the #ifndef CONFIG_USER_ONLY to entire else of
>     if (cpl != 0).
> 
> * i386: split misc helper into user and sysemu parts
>   - added g_assert_not_reached() and changed user file name to -stubs.
> 
> * i386: separate fpu_helper into user and sysemu parts
>   - removed unused return value
>   - added comment abut issues with current cpu_x86_fsave.
> 
> * i386: split off sysemu part of cpu.c
>   - rename cpu-softmmu.c to cpu-sysemu.c
>   - fixed two mispelled comments, and add two comments
>     in the headers of cpu.c and cpu-sysemu.c to describe them
> 
> * i386: gdbstub: only write CR0/CR2/CR3/EFER for sysemu
>   - defined some aux functions to reduce repeated code
> 
> * i386: make cpu_load_efer sysemu-only
>   - move the function to helper.c, remove "inline"
> 
> v21 -> v22: replace "softmmu" with "sysemu"
> 
> v20 -> v21:
> 
> * meson: add target_user_arch
>    - add hexagon
> 
> v19 -> v20:
> 
> * add new patch to make gdbstub only write certain registers for softmmu.
>    In particular, CR0, CR2, CR3 and EFER should not be changed under
>    CONFIG_USER_ONLY. (Paolo)
> 
> * add new patch to make cpu_load_efer softmmu-only (Paolo)
> 
> * i386: split svm_helper into softmmu and stub-only user
> 
>    - fixed commit message spelling (Eric)
> 
>    - mention in commit message that this reproduces the existing stubs,
>      but really everything that requires s->cpl == 0 should be impossible
>      to trigger from user-mode, and we could find a way to assert that
>      more consistently.
> 
> v18 -> v19:
> 
> * i386: split smm helper (softmmu)
>    - add g_assert_not_reached and cpu_abort for invalid states in
>      CONFIG_USER_ONLY (Paolo)
> 
> * i386: move TCG btp_helper into softmmu/
>    - for CONFIG_USER_ONLY, assert that the hidden IOBPT flags are not set
>      while attempting to generate io_bpt helpers.
>      Theory to verify (Paolo)
> 
> * i386: slit svm_helper into softmmu and stub-only user
>    - added XXX in the commit message to highlight the question about
>      whether the same check should be done controlling access to
>      cpu_load_efer() and state of the hidden SVME flag. (Paolo)
> 
> v17 -> v18:
> 
> * meson: add target_user_arch
> 
>   - add target_user_arch to all targets which build user.
>     Otherwise meson complains about missing key for archs without it.
>     (Paolo)
> 
> * wrap a few gen_helper_ calls around ifndef CONFIG_USER_ONLY.
>    This would need a look from someone like Alex or Richard I think,
>    as potentially we could remove even more code I think around the
>    gen_helper_ calls for CONFIG_USER_ONLY.
> 
>    In the current master code, we have empty helpers for user mode,
>    but still we generate the preamble code, temporary variables etc,
>    just to then call a helper_() function that does nothing.
> 
>    In particular I am referring to patches:
> 
>    i386: split tcg btp_helper into softmmu and user parts
>          DEF_HELPER_FLAGS_3(set_dr, TCG_CALL_NO_WG, void, env, int, tl)
>          DEF_HELPER_FLAGS_4(bpt_io, TCG_CALL_NO_WG, void, env, i32, i32, tl)
>          gen_bpt_io
>          gen_helper_set_dr(cpu_env, s->tmp2_i32, s->T0);
> 
>    i386: split smm helper (softmmu)
>          DEF_HELPER_1(rsm, void, env)
> 	gen_helper_rsm(cpu_env);
> 
>    (Alex, Richard?)
> 
> * removed suffixes from user/ and softmmu/ modules
>    (Alex, Philippe).
>    Where possible, removed user stubs entirely.
>    Renamed the leftover svm_helper stubs to user/svm_stubs.c
> 
> * cleaned up lefover unnecessary header files and squashed them.
>   
> 
> v16 -> v17: changed to RFC
> 
> * tcg_ops are already in master, removed from the series
> 
> * i386: split cpu accelerators from cpu.c, using AccelCPUClass:
>    removed spurious ; and added spacing before/after functions (Richard)
> 
> * added new patches as RFC for the next steps, introducing target-specific
>    user-mode specific meson variables, and applied to i386/tcg as an
>    example, in order to gather feedback.
> 
> v15 -> v16:
> 
> * cpu: Move synchronize_from_tb() to tcg_ops:
>    - adjusted comments (Alex)
> 
> * cpu: tcg_ops: move to tcg-cpu-ops.h, keep a pointer in CPUClass:
>    - remove forward decl. of AccelCPUClass, should be in a later patch. (Alex)
>    - simplified comment about tcg_ops in struct CPUClass (Alex)
>    - remove obsolete comment about ARM blocking TCGCPUOps from being const.
>      (Alex)
> 
> * accel: replace struct CpusAccel with AccelOpsClass:
>    - reworded commit message to be clearer about the objective (Alex)
> 
> * accel: introduce AccelCPUClass extending CPUClass
>    - reworded commit message to be clearer about the objective (Alex)
> 
> * hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn:
>    - dropped this patch (Alex, Philippe)
> 
>    will try again later, also in the context of:
>    https://www.mail-archive.com/qemu-devel@nongnu.org/msg686480.html
> 
> * accel: introduce new accessor functions
>    - squashed comments in previous patch introducing accel-cpu.h. (Philippe)
> 
> * accel-cpu: make cpu_realizefn return a bool
>    - split in two patches, separating the change to the phys_bits check
>      (Philippe)
> 
> v14 -> v15:
> 
> * change the TcgCpuOperations so that all fields of the struct are
>    defined unconditionally, as per original patch by Eduardo,
>    before moving them to a separate struct referenced by a pointer
>    (Richard, Eduardo).
> 
> * changed (c) year to 2021
> 
> * added a patch to make accel_cpu->cpu_realizefn return bool, and
>    adapt host_cpu, kvm_cpu, hvf_cpu and tcg_cpu in i386 accordingly.
>    Ultimately, consistently moving to a pattern of realize functions
>    returning bool will require a rework of all devices.
> 
> v13 -> v14: rebased on latest master.
> v12 -> v13: rebased on latest master.
> 
> v11 -> v12: reordered patches and improved tcg_ops
> 
> * reordered all TcgCpuOperations stuff so it is at the beginning
> 
> * added patches for ARM-specific tcg ops
>    debug_check_watchpoint and adjust_watchpoint_address
> 
> * added a patch that puts a forward declared pointer in the struct,
>    so as to reduce the change of misuse between common_ss and specific_ss code,
>    and tidy up as a consequence all targets, by defining dedicated structs.
> 
> v10 -> v11: split off PART 2,
> 
> no further changes to PART 2 other than the split.
> 
> v9 -> v10: minor tweaks and fixes
> 
> * in "i386: split cpu accelerators from cpu.c",
> 
> use kvm/kvm-cpu.c, hvf/hvf-cpu.c, tcg/tcg-cpu.c.
> Easier to understand compared to editing multiple cpu.c files,
> and matches the header files if needed (kvm-cpu.h).
> 
> * in "accel: replace struct CpusAccel with AccelOpsClass",
> 
> make it a bit more consistent, by naming the files defining
> the AccelOpsClass types "...-accel-ops.c" instead of the old
> naming "...-cpus.c".
> 
> * in "cpu: move cc->transaction_failed to tcg_ops",
> 
> protect with CONFIG_TCG the use of tcg_ops for hw/misc/jazz.c,
> 
>   #include "exec/memattrs.h" (Philippe, Eduardo)
> 
> * in "cpu: Move synchronize_from_tb() to tcg_ops",
> 
>   #include "hw/core/cpu.h" (Philippe, Eduardo)
> 
> do not remove the comment about struct TcgCpuOperations (Philippe)
> 
> * in "accel/tcg: split TCG-only code from cpu_exec_realizefn",
> 
> invert tcg_target_initialized set order (Alex)
> 
> * in "i386: move TCG cpu class initialization out of helper.c",
> 
> extract helper-tcg.h, tcg-cpu.c, and tcg-cpu.h directly into
> tcg/, avoiding the extra move later to tcg/ (Alex)
> 
> 
> 
> v8 -> v9: move additional methods to CPUClass->tcg_ops
> 
> do_unaligned_access, transaction_failed and do_interrupt.
> 
> do_interrupt is a bit tricky, as the same code is reused
> (albeit not usually directly) for KVM under certain odd conditions.
> 
> Change arm, as the only user of do_interrupt callback for KVM,
> to instead call the target function directly arm_do_interrupt.
> 
> v7 -> v8: add missing CONFIG_TCGs, fix bugs
> 
> * add the prerequisite patches for "3 tcg" at the beginning of the
>    series for convenience (already reviewed, queued by RH).
> 
> * add CONFIG_TCG to TCGCpuOperations and tcg_ops variable use
> 
> * reduce the scope of the realizefn refactoring, do not
>    introduce a separate cpu_accel_realize, and instead use the
>    existing cpu_exec_realizefn, there is not enough benefit
>    to introduce a new function.
> 
> * fix bugs in user mode due to attempt to move the tcg_region_init()
>    early, so it could be done just once in tcg_init() for both
>    softmmu and user mode. Unfortunately it needs to remain deferred
>    for user mode, as it needs to be done after prologue init and
>    after the GUEST_BASE has been set.
> 
> v6 -> v7: integrate TCGCpuOperations, refactored cpu_exec_realizefn
> 
> * integrate TCGCpuOperations (Eduardo)
> 
> Taken some refactoring from Eduardo for Tcg-only operations on
> CPUClass.
> 
> * refactored cpu_exec_realizefn
> 
> The other main change is a refactoring of cpu_exec_realizefn,
> directly linked to the effort of making many cpu_exec operations
> TCG-only (Eduardo series above):
> 
> cpu_exec_realizefn is actually a TCG-only thing, with the
> exception of a couple things that can be done in base cpu code.
> 
> This changes all targets realizefn, so I guess I have to Cc:
> the Multiverse? (Universe was already CCed for all accelerators).
> 
> 
> v5 -> v6: remove MODULE_INIT_ACCEL_CPU
> 
> 
> instead, use a call to accel_init_interfaces().
> 
> * The class lookups are now general and performed in accel/
> 
>    new AccelCPUClass for new archs are supported as new
>    ones appear in the class hierarchy, no need for stubs.
> 
> * Split the code a bit better
> 
> 
> v4 -> v5: centralized and simplified initializations
> 
> I put in Cc: Emilio G. Cota, specifically because in patch 8
> I (re)moved for user-mode the call to tcg_regions_init().
> 
> The call happens now inside the tcg AccelClass machine_init,
> (so earlier). This seems to work fine, but thought to get the
> author opinion on this.
> 
> Rebased on "tcg-cpus: split into 3 tcg variants" series
> (queued by Richard), to avoid some code churn:
> 
> 
> https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg04356.html
> 
> 
> * Extended AccelClass to user-mode.
> 
> user-mode now does not call tcg_exec_init directly,
> instead it uses the tcg accel class, and its init_machine method.
> 
> Since user-mode does not define or use a machine state,
> the machine is just passed as NULL.
> 
> The immediate advantage is that now we can call current_accel()
> from both user mode and softmmu, so we can work out the correct
> class to use for accelerator initializations.
> 
> * QOMification of CpusAccelOps
> 
> simple QOMification of CpusAccelOps abstract class.
> 
> * Centralized all accel_cpu_init, so only one per cpu-arch,
>    plus one for all accels will remain.
> 
>    So we can expect accel_cpu_init() to be limited to:
>    
>    softmmu/cpus.c - initializes the chosen softmmu accel ops for the cpus module.
>    target/ARCH/cpu.c - initializes the chosen arch-specific cpu accelerator.
>    
> These changes are meant to address concerns/issues (Paolo):
> 
> 1) the use of if (tcg_enabled()) and similar in the module_init call path
> 
> 2) the excessive number of accel_cpu_init() to hunt down in the codebase.
> 
> 
> * Fixed wrong use of host_cpu_class_init (Eduardo)
> 
> 
> v3 -> v4: QOMification of X86CPUAccelClass
> 
> 
> In this version I basically QOMified X86CPUAccel, taking the
> suggestions from Eduardo as the starting point,
> but stopping just short of making it an actual QOM interface,
> using a plain abstract class, and then subclasses for the
> actual objects.
> 
> Initialization is still using the existing qemu initialization
> framework (module_call_init), which is I still think is better
> than the alternatives proposed, in the current state.
> 
> Possibly some improvements could be developed in the future here.
> In this case, effort should be put in keeping things extendible,
> in order not to be blocked once accelerators also become modules.
> 
> Motivation and higher level steps:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html
> 
> Looking forward to your comments on this proposal,
> 
> Ciao,
> 
> Claudio
> 
> 
> Claudio Fontana (21):
>    i386: split cpu accelerators from cpu.c, using AccelCPUClass
>    cpu: call AccelCPUClass::cpu_realizefn in cpu_exec_realizefn
>    accel: introduce new accessor functions
>    target/i386: fix host_cpu_adjust_phys_bits error handling
>    accel-cpu: make cpu_realizefn return a bool
>    meson: add target_user_arch
>    i386: split off sysemu-only functionality in tcg-cpu
>    i386: split smm helper (sysemu)
>    i386: split tcg excp_helper into sysemu and user parts
>    i386: move TCG bpt_helper into sysemu/
>    i386: split misc helper user stubs and sysemu part
>    i386: separate fpu_helper sysemu-only parts
>    i386: split svm_helper into sysemu and stub-only user
>    i386: split seg_helper into user-only and sysemu parts
>    i386: split off sysemu part of cpu.c
>    target/i386: gdbstub: introduce aux functions to read/write CS64 regs
>    target/i386: gdbstub: only write CR0/CR2/CR3/EFER for sysemu
>    i386: make cpu_load_efer sysemu-only
>    accel: move call to accel_init_interfaces
>    accel: add init_accel_cpu for adapting accel behavior to CPU type
>    XXX RFC accel: add cpu_reset
> 
> Richard Henderson (2):
>    target/i386: Rename helper_fldt, helper_fstt
>    target/i386: Split out do_fsave, do_frstor, do_fxsave, do_fxrstor
> 
>   meson.build                               |   5 +
>   include/hw/core/accel-cpu.h               |   4 +-
>   include/hw/core/cpu.h                     |   6 +
>   include/qemu/accel.h                      |  19 +
>   target/i386/cpu-internal.h                |  70 ++
>   target/i386/cpu.h                         |  43 +-
>   target/i386/helper.h                      |  11 +
>   target/i386/host-cpu.h                    |  19 +
>   target/i386/kvm/kvm-cpu.h                 |  41 ++
>   target/i386/tcg/helper-tcg.h              |   8 +
>   target/i386/tcg/seg_helper.h              |  66 ++
>   target/i386/tcg/tcg-cpu.h                 |  21 +-
>   accel/accel-common.c                      |  41 ++
>   bsd-user/main.c                           |   2 +-
>   cpu.c                                     |   5 +-
>   hw/core/cpu.c                             |   3 +-
>   hw/core/machine.c                         |   1 +
>   hw/i386/pc_piix.c                         |   1 +
>   linux-user/main.c                         |   2 +-
>   softmmu/vl.c                              |   1 -
>   target/i386/cpu-sysemu.c                  | 352 ++++++++++
>   target/i386/cpu.c                         | 779 ++--------------------
>   target/i386/gdbstub.c                     | 165 ++---
>   target/i386/helper.c                      |  13 +
>   target/i386/host-cpu.c                    | 204 ++++++
>   target/i386/hvf/hvf-cpu.c                 |  68 ++
>   target/i386/kvm/kvm-cpu.c                 | 157 +++++
>   target/i386/kvm/kvm.c                     |   3 +-
>   target/i386/tcg/bpt_helper.c              | 276 --------
>   target/i386/tcg/excp_helper.c             | 573 ----------------
>   target/i386/tcg/fpu_helper.c              | 122 ++--
>   target/i386/tcg/misc_helper.c             | 467 -------------
>   target/i386/tcg/seg_helper.c              | 237 +------
>   target/i386/tcg/sysemu/bpt_helper.c       | 293 ++++++++
>   target/i386/tcg/sysemu/excp_helper.c      | 582 ++++++++++++++++
>   target/i386/tcg/sysemu/fpu_helper.c       |  57 ++
>   target/i386/tcg/sysemu/misc_helper.c      | 442 ++++++++++++
>   target/i386/tcg/sysemu/seg_helper.c       | 125 ++++
>   target/i386/tcg/{ => sysemu}/smm_helper.c |  19 +-
>   target/i386/tcg/{ => sysemu}/svm_helper.c |  62 +-
>   target/i386/tcg/sysemu/tcg-cpu.c          |  83 +++
>   target/i386/tcg/tcg-cpu.c                 |  56 +-
>   target/i386/tcg/translate.c               |  13 +-
>   target/i386/tcg/user/excp_helper.c        |  39 ++
>   target/i386/tcg/user/misc_stubs.c         |  75 +++
>   target/i386/tcg/user/seg_helper.c         | 109 +++
>   target/i386/tcg/user/svm_stubs.c          |  76 +++
>   MAINTAINERS                               |   2 +-
>   target/alpha/meson.build                  |   3 +
>   target/arm/meson.build                    |   2 +
>   target/cris/meson.build                   |   3 +
>   target/hexagon/meson.build                |   3 +
>   target/hppa/meson.build                   |   3 +
>   target/i386/hvf/meson.build               |   1 +
>   target/i386/kvm/meson.build               |   7 +-
>   target/i386/meson.build                   |   9 +-
>   target/i386/tcg/meson.build               |   5 +-
>   target/i386/tcg/sysemu/meson.build        |  10 +
>   target/i386/tcg/user/meson.build          |   6 +
>   target/m68k/meson.build                   |   3 +
>   target/microblaze/meson.build             |   3 +
>   target/mips/meson.build                   |   2 +
>   target/nios2/meson.build                  |   3 +
>   target/openrisc/meson.build               |   3 +
>   target/ppc/meson.build                    |   3 +
>   target/riscv/meson.build                  |   3 +
>   target/s390x/meson.build                  |   3 +
>   target/sh4/meson.build                    |   3 +
>   target/sparc/meson.build                  |   3 +
>   target/tricore/meson.build                |   3 +
>   target/xtensa/meson.build                 |   3 +
>   71 files changed, 3321 insertions(+), 2584 deletions(-)
>   create mode 100644 target/i386/cpu-internal.h
>   create mode 100644 target/i386/host-cpu.h
>   create mode 100644 target/i386/kvm/kvm-cpu.h
>   create mode 100644 target/i386/tcg/seg_helper.h
>   create mode 100644 target/i386/cpu-sysemu.c
>   create mode 100644 target/i386/host-cpu.c
>   create mode 100644 target/i386/hvf/hvf-cpu.c
>   create mode 100644 target/i386/kvm/kvm-cpu.c
>   create mode 100644 target/i386/tcg/sysemu/bpt_helper.c
>   create mode 100644 target/i386/tcg/sysemu/excp_helper.c
>   create mode 100644 target/i386/tcg/sysemu/fpu_helper.c
>   create mode 100644 target/i386/tcg/sysemu/misc_helper.c
>   create mode 100644 target/i386/tcg/sysemu/seg_helper.c
>   rename target/i386/tcg/{ => sysemu}/smm_helper.c (98%)
>   rename target/i386/tcg/{ => sysemu}/svm_helper.c (96%)
>   create mode 100644 target/i386/tcg/sysemu/tcg-cpu.c
>   create mode 100644 target/i386/tcg/user/excp_helper.c
>   create mode 100644 target/i386/tcg/user/misc_stubs.c
>   create mode 100644 target/i386/tcg/user/seg_helper.c
>   create mode 100644 target/i386/tcg/user/svm_stubs.c
>   create mode 100644 target/i386/tcg/sysemu/meson.build
>   create mode 100644 target/i386/tcg/user/meson.build
>