mbox series

[RFC,v18,00/15] i386 cleanup PART 2

Message ID 20210212123622.15834-1-cfontana@suse.de (mailing list archive)
Headers show
Series i386 cleanup PART 2 | expand

Message

Claudio Fontana Feb. 12, 2021, 12:36 p.m. UTC
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 (15):
  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 softmmu-only functionality in tcg-cpu
  i386: split smm helper (softmmu)
  i386: split tcg excp_helper into softmmu and user parts
  i386: split tcg btp_helper into softmmu and user parts
  i386: split misc helper into user and softmmu parts
  i386: separate fpu_helper into user and softmmu parts
  i386: slit svm_helper into softmmu and stub-only user
  i386: split seg_helper into user-only and softmmu parts
  i386: split off softmmu part of cpu.c

 meson.build                                |   5 +
 include/hw/core/accel-cpu.h                |   2 +-
 include/qemu/accel.h                       |  13 +
 target/i386/cpu-internal.h                 |  70 ++
 target/i386/cpu.h                          |  23 +-
 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                       |  19 +
 cpu.c                                      |   5 +-
 hw/i386/pc_piix.c                          |   1 +
 target/i386/cpu-softmmu.c                  | 352 ++++++++++
 target/i386/cpu.c                          | 780 ++-------------------
 target/i386/host-cpu.c                     | 204 ++++++
 target/i386/hvf/hvf-cpu.c                  |  68 ++
 target/i386/kvm/kvm-cpu.c                  | 151 ++++
 target/i386/kvm/kvm.c                      |   3 +-
 target/i386/tcg/bpt_helper.c               | 276 --------
 target/i386/tcg/excp_helper.c              | 572 ---------------
 target/i386/tcg/fpu_helper.c               |  65 +-
 target/i386/tcg/misc_helper.c              | 463 ------------
 target/i386/tcg/seg_helper.c               | 235 +------
 target/i386/tcg/softmmu/bpt_helper.c       | 293 ++++++++
 target/i386/tcg/softmmu/excp_helper.c      | 562 +++++++++++++++
 target/i386/tcg/softmmu/fpu_helper.c       |  58 ++
 target/i386/tcg/softmmu/misc_helper.c      | 438 ++++++++++++
 target/i386/tcg/softmmu/seg_helper.c       | 125 ++++
 target/i386/tcg/{ => softmmu}/smm_helper.c |  19 +-
 target/i386/tcg/{ => softmmu}/svm_helper.c |  62 +-
 target/i386/tcg/softmmu/tcg-cpu.c          |  83 +++
 target/i386/tcg/tcg-cpu.c                  |  50 +-
 target/i386/tcg/translate.c                |   6 +
 target/i386/tcg/user/excp_helper.c         |  39 ++
 target/i386/tcg/user/fpu_helper.c          |  42 ++
 target/i386/tcg/user/misc_helper.c         |  72 ++
 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/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/softmmu/meson.build        |  10 +
 target/i386/tcg/user/meson.build           |   7 +
 target/m68k/meson.build                    |   3 +
 target/microblaze/meson.build              |   3 +
 target/mips/meson.build                    |   3 +
 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/tilegx/meson.build                  |   3 +
 target/tricore/meson.build                 |   3 +
 target/xtensa/meson.build                  |   3 +
 64 files changed, 3148 insertions(+), 2450 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-softmmu.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/softmmu/bpt_helper.c
 create mode 100644 target/i386/tcg/softmmu/excp_helper.c
 create mode 100644 target/i386/tcg/softmmu/fpu_helper.c
 create mode 100644 target/i386/tcg/softmmu/misc_helper.c
 create mode 100644 target/i386/tcg/softmmu/seg_helper.c
 rename target/i386/tcg/{ => softmmu}/smm_helper.c (98%)
 rename target/i386/tcg/{ => softmmu}/svm_helper.c (96%)
 create mode 100644 target/i386/tcg/softmmu/tcg-cpu.c
 create mode 100644 target/i386/tcg/user/excp_helper.c
 create mode 100644 target/i386/tcg/user/fpu_helper.c
 create mode 100644 target/i386/tcg/user/misc_helper.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/softmmu/meson.build
 create mode 100644 target/i386/tcg/user/meson.build

Comments

no-reply@patchew.org Feb. 12, 2021, 12:57 p.m. UTC | #1
Patchew URL: https://patchew.org/QEMU/20210212123622.15834-1-cfontana@suse.de/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210212123622.15834-1-cfontana@suse.de
Subject: [RFC v18 00/15] i386 cleanup PART 2

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210211232835.2608059-1-crosa@redhat.com -> patchew/20210211232835.2608059-1-crosa@redhat.com
 * [new tag]         patchew/20210212123622.15834-1-cfontana@suse.de -> patchew/20210212123622.15834-1-cfontana@suse.de
Switched to a new branch 'test'
ed48894 i386: split off softmmu part of cpu.c
dd2e304 i386: split seg_helper into user-only and softmmu parts
3e629d7 i386: slit svm_helper into softmmu and stub-only user
cb66e6a i386: separate fpu_helper into user and softmmu parts
1f6097b i386: split misc helper into user and softmmu parts
510ce76 i386: split tcg btp_helper into softmmu and user parts
a3e6766 i386: split tcg excp_helper into softmmu and user parts
d4748e9 i386: split smm helper (softmmu)
9048a77 i386: split off softmmu-only functionality in tcg-cpu
5ebedb2 meson: add target_user_arch
32174a9 accel-cpu: make cpu_realizefn return a bool
4d3e346 target/i386: fix host_cpu_adjust_phys_bits error handling
d14cb3c accel: introduce new accessor functions
f121c98 cpu: call AccelCPUClass::cpu_realizefn in cpu_exec_realizefn
c63d0f7 i386: split cpu accelerators from cpu.c, using AccelCPUClass

=== OUTPUT BEGIN ===
1/15 Checking commit c63d0f75db32 (i386: split cpu accelerators from cpu.c, using AccelCPUClass)
WARNING: line over 80 characters
#1335: FILE: target/i386/tcg/tcg-cpu.c:125:
+    memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, cpu->cpu_as_mem, 0);

total: 0 errors, 1 warnings, 1256 lines checked

Patch 1/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/15 Checking commit f121c98d2a7b (cpu: call AccelCPUClass::cpu_realizefn in cpu_exec_realizefn)
3/15 Checking commit d14cb3c31355 (accel: introduce new accessor functions)
4/15 Checking commit 4d3e3462a311 (target/i386: fix host_cpu_adjust_phys_bits error handling)
5/15 Checking commit 32174a937139 (accel-cpu: make cpu_realizefn return a bool)
6/15 Checking commit 5ebedb2a2fe7 (meson: add target_user_arch)
7/15 Checking commit 9048a775db0f (i386: split off softmmu-only functionality in tcg-cpu)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#30: 
new file mode 100644

WARNING: line over 80 characters
#115: FILE: target/i386/tcg/softmmu/tcg-cpu.c:72:
+    memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, cpu->cpu_as_mem, 0);

total: 0 errors, 2 warnings, 212 lines checked

Patch 7/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
8/15 Checking commit d4748e97ef3d (i386: split smm helper (softmmu))
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#18: 
 target/i386/tcg/{ => softmmu}/smm_helper.c | 19 ++-----------------

total: 0 errors, 1 warnings, 77 lines checked

Patch 8/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
9/15 Checking commit a3e67665263e (i386: split tcg excp_helper into softmmu and user parts)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#598: 
new file mode 100644

WARNING: Block comments use a leading /* on a separate line
#736: FILE: target/i386/tcg/softmmu/excp_helper.c:134:
+            /* Bits 20-13 provide bits 39-32 of the address, bit 21 is reserved.

WARNING: Block comments use a leading /* on a separate line
#810: FILE: target/i386/tcg/softmmu/excp_helper.c:208:
+/* return value:

WARNING: line over 80 characters
#911: FILE: target/i386/tcg/softmmu/excp_helper.c:309:
+            pdpe_addr = ((pml4e & PG_ADDRESS_MASK) + (((addr >> 30) & 0x1ff) << 3)) &

WARNING: Block comments use a leading /* on a separate line
#1005: FILE: target/i386/tcg/softmmu/excp_helper.c:403:
+            /* Bits 20-13 provide bits 39-32 of the address, bit 21 is reserved.

WARNING: Block comments use a leading /* on a separate line
#1100: FILE: target/i386/tcg/softmmu/excp_helper.c:498:
+        /* only set write access if already dirty... otherwise wait

WARNING: Block comments use * on subsequent lines
#1101: FILE: target/i386/tcg/softmmu/excp_helper.c:499:
+        /* only set write access if already dirty... otherwise wait
+           for dirty access */

WARNING: Block comments use a trailing */ on a separate line
#1101: FILE: target/i386/tcg/softmmu/excp_helper.c:499:
+           for dirty access */

WARNING: Block comments use a leading /* on a separate line
#1114: FILE: target/i386/tcg/softmmu/excp_helper.c:512:
+    /* Even if 4MB pages, we map only one 4KB page in the cache to

WARNING: Block comments use * on subsequent lines
#1115: FILE: target/i386/tcg/softmmu/excp_helper.c:513:
+    /* Even if 4MB pages, we map only one 4KB page in the cache to
+       avoid filling it too fast */

WARNING: Block comments use a trailing */ on a separate line
#1115: FILE: target/i386/tcg/softmmu/excp_helper.c:513:
+       avoid filling it too fast */

ERROR: braces {} are necessary for all arms of this statement
#1129: FILE: target/i386/tcg/softmmu/excp_helper.c:527:
+    if (is_user)
[...]

total: 1 errors, 11 warnings, 612 lines checked

Patch 9/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

10/15 Checking commit 510ce76bff6f (i386: split tcg btp_helper into softmmu and user parts)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#359: 
new file mode 100644

WARNING: Block comments use a leading /* on a separate line
#495: FILE: target/i386/tcg/softmmu/bpt_helper.c:132:
+    /* If nothing is changing except the global/local enable bits,

WARNING: Block comments use * on subsequent lines
#496: FILE: target/i386/tcg/softmmu/bpt_helper.c:133:
+    /* If nothing is changing except the global/local enable bits,
+       then we can make the change more efficient.  */

WARNING: Block comments use a trailing */ on a separate line
#496: FILE: target/i386/tcg/softmmu/bpt_helper.c:133:
+       then we can make the change more efficient.  */

WARNING: Block comments use a leading /* on a separate line
#498: FILE: target/i386/tcg/softmmu/bpt_helper.c:135:
+        /* Fold the global and local enable bits together into the

WARNING: Block comments use * on subsequent lines
#499: FILE: target/i386/tcg/softmmu/bpt_helper.c:136:
+        /* Fold the global and local enable bits together into the
+           global fields, then xor to show which registers have

WARNING: Block comments use a trailing */ on a separate line
#500: FILE: target/i386/tcg/softmmu/bpt_helper.c:137:
+           changed collective enable state.  */

total: 0 errors, 7 warnings, 616 lines checked

Patch 10/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
11/15 Checking commit 1f6097bf8134 (i386: split misc helper into user and softmmu parts)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#526: 
new file mode 100644

ERROR: switch and case should be at the same indent
#819: FILE: target/i386/tcg/softmmu/misc_helper.c:289:
+    switch ((uint32_t)env->regs[R_ECX]) {
[...]
+     case MSR_IA32_UCODE_REV:

total: 1 errors, 1 warnings, 1009 lines checked

Patch 11/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

12/15 Checking commit cb66e6ae7b97 (i386: separate fpu_helper into user and softmmu parts)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#142: 
new file mode 100644

total: 0 errors, 1 warnings, 219 lines checked

Patch 12/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
13/15 Checking commit 3e629d77855d (i386: slit svm_helper into softmmu and stub-only user)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#12: 
 target/i386/tcg/{ => softmmu}/svm_helper.c | 62 +-----------------------

total: 0 errors, 1 warnings, 169 lines checked

Patch 13/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
14/15 Checking commit dd2e304724ac (i386: split seg_helper into user-only and softmmu parts)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#320: 
new file mode 100644

ERROR: do not use C99 // comments
#347: FILE: target/i386/tcg/seg_helper.h:23:
+//#define DEBUG_PCALL

WARNING: Block comments use a leading /* on a separate line
#620: FILE: target/i386/tcg/user/seg_helper.c:73:
+    /* Since we emulate only user space, we cannot do more than

WARNING: Block comments use * on subsequent lines
#621: FILE: target/i386/tcg/user/seg_helper.c:74:
+    /* Since we emulate only user space, we cannot do more than
+       exiting the emulation with the suitable exception and error

WARNING: Block comments use a trailing */ on a separate line
#622: FILE: target/i386/tcg/user/seg_helper.c:75:
+       code. So update EIP for INT 0x80 and EXCP_SYSCALL. */

WARNING: Block comments use a leading /* on a separate line
#633: FILE: target/i386/tcg/user/seg_helper.c:86:
+    /* if user mode only, we simulate a fake exception

WARNING: Block comments use * on subsequent lines
#634: FILE: target/i386/tcg/user/seg_helper.c:87:
+    /* if user mode only, we simulate a fake exception
+       which will be handled outside the cpu execution

WARNING: Block comments use a trailing */ on a separate line
#635: FILE: target/i386/tcg/user/seg_helper.c:88:
+       loop */

total: 1 errors, 7 warnings, 595 lines checked

Patch 14/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

15/15 Checking commit ed4889490139 (i386: split off softmmu part of cpu.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#17: 
new file mode 100644

WARNING: Block comments use a leading /* on a separate line
#51: FILE: target/i386/cpu-internal.h:30:
+    /* feature flags names are taken from "Intel Processor Identification and

WARNING: Block comments use a leading /* on a separate line
#134: FILE: target/i386/cpu-softmmu.c:37:
+/* Return a QDict containing keys for all properties that can be included

WARNING: Block comments use a leading /* on a separate line
#187: FILE: target/i386/cpu-softmmu.c:90:
+/* Convert CPU model data from X86CPU object to a property dictionary

WARNING: Block comments use a leading /* on a separate line
#201: FILE: target/i386/cpu-softmmu.c:104:
+/* Convert CPU model data from X86CPU object to a property dictionary

WARNING: Block comments use a leading /* on a separate line
#217: FILE: target/i386/cpu-softmmu.c:120:
+        /* "hotplugged" is the only property that is configurable

WARNING: Block comments use a leading /* on a separate line
#307: FILE: target/i386/cpu-softmmu.c:210:
+        /* As we don't return every single property, full expansion needs

total: 0 errors, 7 warnings, 895 lines checked

Patch 15/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210212123622.15834-1-cfontana@suse.de/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Alex Bennée Feb. 15, 2021, 11:37 a.m. UTC | #2
Claudio Fontana <cfontana@suse.de> writes:

<snip>
> Looking forward to your comments on this proposal,
>
<snip>

So I've reviewed as much as I'm comfortable with - I'm going to defer to
the x86 experts on the split of stuff for x86. However from my point of
view I think it's a nice step in improving modularity and reducing the
maze of #ifdefs in the code.
Claudio Fontana Feb. 15, 2021, 11:48 a.m. UTC | #3
On 2/15/21 12:37 PM, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
> <snip>
>> Looking forward to your comments on this proposal,
>>
> <snip>
> 
> So I've reviewed as much as I'm comfortable with - I'm going to defer to
> the x86 experts on the split of stuff for x86. However from my point of
> view I think it's a nice step in improving modularity and reducing the
> maze of #ifdefs in the code.
> 

Thanks a lot Alex for your review.

If I can leverage your TCG knowledge a bit more, as I forgot a lot about it,

have you noticed in "i386: split tcg btp_helper into softmmu and user parts"
and "i386: split smm helper (softmmu)"

those new

#ifndef CONFIG_USER_ONLY?

Basically we remove some preamble stuff that would end up calling empty stubs for user mode,
but do you think there could be some other consequence I am not seeing?

Maybe there is even more that could be removed in the code immediately preceding those CONFIG_USER_ONLY?

In particular I am referring to patches:

  i386: split tcg btp_helper into softmmu and user parts
  i386: split smm helper (softmmu)

I am commenting those patches now inline, so that it is easier to see what I am talking about.

Again, thanks a lot!

Claudio