| Message ID | 20220306130000.8104-1-philippe.mathieu.daude@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 6 Mar 2022 at 13:03, Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> wrote: > > From: Philippe Mathieu-Daudé <f4bug@amsat.org> > > The following changes since commit 9d662a6b22a0838a85c5432385f35db2488a33a5: > > Merge remote-tracking branch 'remotes/legoater/tags/pull-ppc-20220305' into staging (2022-03-05 18:03:15 +0000) > > are available in the Git repository at: > > https://github.com/philmd/qemu.git tags/abstract-arch-cpu-20220306 > > for you to fetch changes up to 5bbf37aa89881751828d28e38608db0371874aef: > > accel/tcg: Remove pointless CPUArchState casts (2022-03-06 13:15:42 +0100) > > ---------------------------------------------------------------- > - Re-org accel/ and softmmu/ to have more target-agnostic objects. > > - Use CPUArchState as an abstract type, defined by each target > (CPUState is our interface with generic code, CPUArchState is > our interface with target-specific code). > > ---------------------------------------------------------------- I get a compile failure on my OSX box: In file included from ../../target/i386/hvf/x86.c:24: ../../target/i386/hvf/x86_emu.h:27:30: error: declaration of 'struct CPUX86State' will not be visible outside of this function [-Werror,-Wvisibility] bool exec_instruction(struct CPUX86State *env, struct x86_decode *ins); ^ ../../target/i386/hvf/x86_emu.h:39:27: error: declaration of 'struct CPUX86State' will not be visible outside of this function [-Werror,-Wvisibility] void write_val_ext(struct CPUX86State *env, target_ulong ptr, target_ulong val, int size); ^ [ditto for all the other uses of struct CPUX86State] thanks -- PMM
On 6/3/22 19:16, Peter Maydell wrote: > On Sun, 6 Mar 2022 at 13:03, Philippe Mathieu-Daudé > <philippe.mathieu.daude@gmail.com> wrote: >> >> From: Philippe Mathieu-Daudé <f4bug@amsat.org> >> >> The following changes since commit 9d662a6b22a0838a85c5432385f35db2488a33a5: >> >> Merge remote-tracking branch 'remotes/legoater/tags/pull-ppc-20220305' into staging (2022-03-05 18:03:15 +0000) >> >> are available in the Git repository at: >> >> https://github.com/philmd/qemu.git tags/abstract-arch-cpu-20220306 >> >> for you to fetch changes up to 5bbf37aa89881751828d28e38608db0371874aef: >> >> accel/tcg: Remove pointless CPUArchState casts (2022-03-06 13:15:42 +0100) >> >> ---------------------------------------------------------------- >> - Re-org accel/ and softmmu/ to have more target-agnostic objects. >> >> - Use CPUArchState as an abstract type, defined by each target >> (CPUState is our interface with generic code, CPUArchState is >> our interface with target-specific code). >> >> ---------------------------------------------------------------- > > I get a compile failure on my OSX box: > > In file included from ../../target/i386/hvf/x86.c:24: > ../../target/i386/hvf/x86_emu.h:27:30: error: declaration of 'struct > CPUX86State' will not be visible outside of this function > [-Werror,-Wvisibility] > bool exec_instruction(struct CPUX86State *env, struct x86_decode *ins); > ^ > ../../target/i386/hvf/x86_emu.h:39:27: error: declaration of 'struct > CPUX86State' will not be visible outside of this function > [-Werror,-Wvisibility] > void write_val_ext(struct CPUX86State *env, target_ulong ptr, > target_ulong val, int size); > ^ > [ditto for all the other uses of struct CPUX86State] I see. I only have access to aarch64 Darwin, not x86_64; I was relying on our CI for that (my GitLab CI is green). I'll work a fix, thanks. Phil.
On Sun, 6 Mar 2022 at 19:06, Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> wrote: > I see. I only have access to aarch64 Darwin, not x86_64; I was relying > on our CI for that (my GitLab CI is green). I'll work a fix, thanks. This was on my ad-hoc stuff -- I guess our gitlab CI for macos doesn't build hvf ? -- PMM
+Daniel/Alex On 6/3/22 20:56, Peter Maydell wrote: > On Sun, 6 Mar 2022 at 19:06, Philippe Mathieu-Daudé > <philippe.mathieu.daude@gmail.com> wrote: >> I see. I only have access to aarch64 Darwin, not x86_64; I was relying >> on our CI for that (my GitLab CI is green). I'll work a fix, thanks. > > This was on my ad-hoc stuff -- I guess our gitlab CI for macos > doesn't build hvf ? No, it does: https://gitlab.com/philmd/qemu/-/jobs/2167582776#L6444 Targets and accelerators KVM support : NO HAX support : YES HVF support : YES WHPX support : NO NVMM support : NO Xen support : NO TCG support : YES But the Cirrus job are allowed to fail: .cirrus_build_job: stage: build image: registry.gitlab.com/libvirt/libvirt-ci/cirrus-run:master needs: [] allow_failure: true So I've been missing this error since 2 months... In file included from ../target/i386/hvf/x86_task.c:22: ../target/i386/hvf/x86_emu.h:27:30: error: declaration of 'struct CPUX86State' will not be visible outside of this function [-Werror,-Wvisibility] bool exec_instruction(struct CPUX86State *env, struct x86_decode *ins); ^
On Sun, 6 Mar 2022 at 21:13, Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> wrote: > > +Daniel/Alex > > On 6/3/22 20:56, Peter Maydell wrote: > > On Sun, 6 Mar 2022 at 19:06, Philippe Mathieu-Daudé > > <philippe.mathieu.daude@gmail.com> wrote: > >> I see. I only have access to aarch64 Darwin, not x86_64; I was relying > >> on our CI for that (my GitLab CI is green). I'll work a fix, thanks. > > > > This was on my ad-hoc stuff -- I guess our gitlab CI for macos > > doesn't build hvf ? > > No, it does: > > https://gitlab.com/philmd/qemu/-/jobs/2167582776#L6444 > > Targets and accelerators > KVM support : NO > HAX support : YES > HVF support : YES > WHPX support : NO > NVMM support : NO > Xen support : NO > TCG support : YES > > But the Cirrus job are allowed to fail: Overall I am starting to feel that we should stop having these CI jobs that are in the "allowed to fail" category. All that happens is that they eat a lot of CPU on our CI hosts, but they don't actually find bugs because everybody (rightly) treats "allowed-to-fail-and-failed" as "ignore me". I think our CI jobs should either be "must pass", or else "run only manually", with that latter category being rarely used and only where there's a good reason (eg somebody specific has taken responsibility for debugging some intermittent failure and having it still available in the CI UI for them to trigger is helpful). Plus we really need to get on top of all the intermittent failures. The current state of the world is that we have some intermittents, which makes it easy for new intermittents to get into the tree, because everybody is in the habit of "just hit retry"... thanks -- PMM
On Mon, Mar 07, 2022 at 11:51:20AM +0000, Peter Maydell wrote: > On Sun, 6 Mar 2022 at 21:13, Philippe Mathieu-Daudé > <philippe.mathieu.daude@gmail.com> wrote: > > > > +Daniel/Alex > > > > On 6/3/22 20:56, Peter Maydell wrote: > > > On Sun, 6 Mar 2022 at 19:06, Philippe Mathieu-Daudé > > > <philippe.mathieu.daude@gmail.com> wrote: > > >> I see. I only have access to aarch64 Darwin, not x86_64; I was relying > > >> on our CI for that (my GitLab CI is green). I'll work a fix, thanks. > > > > > > This was on my ad-hoc stuff -- I guess our gitlab CI for macos > > > doesn't build hvf ? > > > > No, it does: > > > > https://gitlab.com/philmd/qemu/-/jobs/2167582776#L6444 > > > > Targets and accelerators > > KVM support : NO > > HAX support : YES > > HVF support : YES > > WHPX support : NO > > NVMM support : NO > > Xen support : NO > > TCG support : YES > > > > But the Cirrus job are allowed to fail: > > Overall I am starting to feel that we should stop having > these CI jobs that are in the "allowed to fail" category. > All that happens is that they eat a lot of CPU on our CI > hosts, but they don't actually find bugs because everybody > (rightly) treats "allowed-to-fail-and-failed" as "ignore me". > I think our CI jobs should either be "must pass", or else > "run only manually", with that latter category being rarely > used and only where there's a good reason (eg somebody > specific has taken responsibility for debugging some > intermittent failure and having it still available in the > CI UI for them to trigger is helpful). The cirrus CI jobs were introduced as allow-fail as we were not sure the cirrus-run integration with gitlab would be entirely stable. There was a blip a month or so ago due to Cirrus CI breaking their REST API, but on the QEMU side we seem to be OK. So I think we can toggle the flag to make these Cirrus CI jobs gating. > Plus we really need to get on top of all the intermittent > failures. The current state of the world is that we have > some intermittents, which makes it easy for new intermittents > to get into the tree, because everybody is in the habit of > "just hit retry"... A big issue IMHO is that the pain/impact hits the wrong people. It is most seriously impacts & disrupts Peter when merging, and less impacts the subsystem maintainers, and even less the original authors. If we consider a alternative world where we used merge requests for subsystem maintainers just to send pull requests. The subsystem maintainer would open a MR and it would be their responsibility to get a green pipeline. Peter (or the person approving pulls for merge at the time) shouldn't even have to consider a MR until it has got a green pipeline. That would put the primary impact of unreliable CI onto the subsystem maintainers, blocking their work from being considered for merge. This creates a direct incentive on the subsystem maintainers to contribute to ensuring reliable CI, instead of considering it somebody else's problem. Regards, Daniel
On Mon, 7 Mar 2022 at 12:12, Daniel P. Berrangé <berrange@redhat.com> wrote: > A big issue IMHO is that the pain/impact hits the wrong people. > It is most seriously impacts & disrupts Peter when merging, and > less impacts the subsystem maintainers, and even less the > original authors. > > If we consider a alternative world where we used merge requests > for subsystem maintainers just to send pull requests. The subsystem > maintainer would open a MR and it would be their responsibility > to get a green pipeline. Peter (or the person approving pulls for > merge at the time) shouldn't even have to consider a MR until it > has got a green pipeline. That would put the primary impact of > unreliable CI onto the subsystem maintainers, blocking their work > from being considered for merge. This creates a direct incentive > on the subsystem maintainers to contribute to ensuring reliable > CI, instead of considering it somebody else's problem. CI fails merge isn't really a big deal IME -- I just bounce the merge request. The real problem and timesink (especially of CI hours) is the intermittents. -- PMM
On Mon, Mar 07, 2022 at 12:17:24PM +0000, Peter Maydell wrote: > On Mon, 7 Mar 2022 at 12:12, Daniel P. Berrangé <berrange@redhat.com> wrote: > > A big issue IMHO is that the pain/impact hits the wrong people. > > It is most seriously impacts & disrupts Peter when merging, and > > less impacts the subsystem maintainers, and even less the > > original authors. > > > > If we consider a alternative world where we used merge requests > > for subsystem maintainers just to send pull requests. The subsystem > > maintainer would open a MR and it would be their responsibility > > to get a green pipeline. Peter (or the person approving pulls for > > merge at the time) shouldn't even have to consider a MR until it > > has got a green pipeline. That would put the primary impact of > > unreliable CI onto the subsystem maintainers, blocking their work > > from being considered for merge. This creates a direct incentive > > on the subsystem maintainers to contribute to ensuring reliable > > CI, instead of considering it somebody else's problem. > > CI fails merge isn't really a big deal IME -- I just bounce > the merge request. The real problem and timesink (especially of > CI hours) is the intermittents. I'm saying the intermittents shouldn't be your problem to babysit either though. The subsystem maintainer should own the whole problem of getting the CI into a green state, either by fixing the genuine bugs in the pull, but also by re-trying the jobs on intermittent failure and/or by fixing the non-deterministic pre-existing problems. The person approving the merge to git master shouldn't have to do more than look at the CI results to check they are green and hit the button to apply. GitLab can even enforce this by disabling the ability to apply the merge request until CI is green, to avoid accidentally merging something with bad CI results. Regards, Daniel
From: Philippe Mathieu-Daudé <f4bug@amsat.org> The following changes since commit 9d662a6b22a0838a85c5432385f35db2488a33a5: Merge remote-tracking branch 'remotes/legoater/tags/pull-ppc-20220305' into staging (2022-03-05 18:03:15 +0000) are available in the Git repository at: https://github.com/philmd/qemu.git tags/abstract-arch-cpu-20220306 for you to fetch changes up to 5bbf37aa89881751828d28e38608db0371874aef: accel/tcg: Remove pointless CPUArchState casts (2022-03-06 13:15:42 +0100) ---------------------------------------------------------------- - Re-org accel/ and softmmu/ to have more target-agnostic objects. - Use CPUArchState as an abstract type, defined by each target (CPUState is our interface with generic code, CPUArchState is our interface with target-specific code). ---------------------------------------------------------------- Philippe Mathieu-Daudé (32): accel: Restrict sysemu stubs to system emulation accel/meson: Only build hw virtualization with system emulation exec: Declare vaddr as a generic target-agnostic type exec: Make cpu_memory_rw_debug() target agnostic sysemu/memory_mapping: Become target-agnostic sysemu/kvm: Make kvm_on_sigbus() / kvm_on_sigbus_vcpu() target agnostic accel/kvm: Simplify user-mode #ifdef'ry accel/hax: Introduce CONFIG_HAX_IS_POSSIBLE softmmu/cpus: Code movement accel: Introduce AccelOpsClass::cpu_thread_is_idle() accel: Introduce AccelOpsClass::cpus_are_resettable() softmmu/globals: Remove unused 'hw/i386/*' headers softmmu/physmem: Remove unnecessary include softmmu/cpu-timers: Remove unused 'exec/exec-all.h' header misc: Remove unnecessary "sysemu/cpu-timers.h" include misc: Add missing "sysemu/cpu-timers.h" include exec/gdbstub: Make gdb_exit() / gdb_set_stop_cpu() target agnostic exec/cpu: Make address_space_init/reloading_memory_map target agnostic softmmu: Add qemu_init_arch_modules() softmmu: Build target-agnostic objects once meson: Display libfdt as disabled when system emulation is disabled exec/cpu_ldst: Include 'cpu.h' to get target_ulong definition cpu: Add missing 'exec/exec-all.h' and 'qemu/accel.h' headers target/i386/tcg/sysemu: Include missing 'exec/exec-all.h' header target: Include missing 'cpu.h' target/hexagon: Add missing 'hw/core/cpu.h' include target: Use forward declared type instead of structure type target: Use CPUArchState as interface to target-specific CPU state target: Introduce and use OBJECT_DECLARE_CPU_TYPE() macro target: Use ArchCPU as interface to target CPU target/i386: Remove pointless CPUArchState casts accel/tcg: Remove pointless CPUArchState casts Taylor Simpson (1): Hexagon (target/hexagon) convert to OBJECT_DECLARE_TYPE meson.build | 4 ++- include/exec/cpu-all.h | 4 --- include/exec/cpu-common.h | 39 ++++++++++++++++++++++++++++ include/exec/cpu_ldst.h | 1 + include/exec/exec-all.h | 26 ------------------- include/exec/gdbstub.h | 25 +++++++++--------- include/exec/poison.h | 2 -- include/hw/core/cpu.h | 33 +++++++++++++---------- include/qemu/typedefs.h | 2 ++ include/sysemu/accel-ops.h | 3 +++ include/sysemu/arch_init.h | 2 ++ include/sysemu/hax.h | 18 ++++++++----- include/sysemu/hw_accel.h | 5 ---- include/sysemu/kvm.h | 6 ++--- include/sysemu/memory_mapping.h | 5 ++-- target/alpha/cpu-qom.h | 3 +-- target/alpha/cpu.h | 11 +++----- target/arm/cpu-qom.h | 3 +-- target/arm/cpu.h | 7 ++--- target/arm/hvf_arm.h | 2 +- target/avr/cpu-qom.h | 3 +-- target/avr/cpu.h | 13 +++------- target/cris/cpu-qom.h | 3 +-- target/cris/cpu.h | 7 ++--- target/hexagon/cpu.h | 23 ++++++---------- target/hppa/cpu-qom.h | 3 +-- target/hppa/cpu.h | 12 +++------ target/i386/cpu-qom.h | 3 +-- target/i386/cpu.h | 7 ++--- target/m68k/cpu-qom.h | 3 +-- target/m68k/cpu.h | 7 ++--- target/microblaze/cpu-qom.h | 3 +-- target/microblaze/cpu.h | 9 +++---- target/microblaze/mmu.h | 2 ++ target/mips/cpu-qom.h | 3 +-- target/mips/cpu.h | 10 +++---- target/mips/internal.h | 15 ++++++----- target/nios2/cpu.h | 9 +++---- target/nios2/mmu.h | 2 ++ target/openrisc/cpu.h | 17 ++++-------- target/ppc/cpu-qom.h | 5 ++-- target/ppc/cpu.h | 7 ++--- target/riscv/cpu.h | 11 +++----- target/riscv/pmp.h | 2 ++ target/rx/cpu-qom.h | 5 +--- target/rx/cpu.h | 6 ++--- target/s390x/cpu-qom.h | 7 +++-- target/s390x/cpu.h | 7 ++--- target/sh4/cpu-qom.h | 3 +-- target/sh4/cpu.h | 7 ++--- target/sparc/cpu-qom.h | 3 +-- target/sparc/cpu.h | 9 +++---- target/tricore/cpu-qom.h | 3 +-- target/tricore/cpu.h | 10 +++---- target/xtensa/cpu-qom.h | 3 +-- target/xtensa/cpu.h | 13 ++++------ accel/kvm/kvm-accel-ops.c | 12 +++++++++ accel/qtest/qtest.c | 1 - accel/stubs/hax-stub.c | 2 ++ accel/stubs/kvm-stub.c | 5 ---- accel/tcg/cpu-exec.c | 4 +-- accel/tcg/tcg-accel-ops-icount.c | 1 + accel/tcg/tcg-accel-ops-mttcg.c | 1 + accel/tcg/tcg-accel-ops-rr.c | 1 + accel/tcg/tcg-accel-ops.c | 1 + cpu.c | 8 +++--- softmmu/arch_init.c | 9 +++++++ softmmu/cpu-timers.c | 1 - softmmu/cpus.c | 23 +++++++++------- softmmu/globals.c | 2 -- softmmu/memory_mapping.c | 1 + softmmu/physmem.c | 7 +++-- softmmu/vl.c | 5 +--- target/alpha/translate.c | 1 - target/i386/hax/hax-all.c | 11 +++----- target/i386/nvmm/nvmm-all.c | 14 +++++----- target/i386/tcg/sysemu/excp_helper.c | 1 + target/i386/tcg/sysemu/misc_helper.c | 1 + target/i386/whpx/whpx-accel-ops.c | 6 +++++ target/i386/whpx/whpx-all.c | 18 ++++++------- target/riscv/csr.c | 1 + tests/unit/ptimer-test-stubs.c | 1 - accel/meson.build | 12 +++++---- accel/stubs/meson.build | 11 +++++--- softmmu/meson.build | 24 ++++++++--------- 85 files changed, 305 insertions(+), 336 deletions(-)