On 07/16/19 22:10, Philippe Mathieu-Daudé wrote: > On 7/16/19 8:42 PM, Laszlo Ersek wrote: >> On 07/16/19 18:59, Peter Maydell wrote: >>> On Tue, 16 Jul 2019 at 17:51, Laszlo Ersek <ler...@redhat.com> wrote: >>>> The issue still reproduces, so it makes sense for me to look at the host >>>> kernel version... Well, I'm afraid it won't help much, for an upstream >>>> investigation: >>>> >>>> 4.14.0-115.8.2.el7a.aarch64 >>>> >>>> This is the latest released kernel from "Red Hat Enterprise Linux for >>>> ARM 64 7". >>> >>> OK. (I'm using 4.15.0-51-generic from ubuntu). >>> >>> Could you run with QEMU under gdb, and when it hits the >>> assertion go back up a stack frame to the arm_cpu_realizefn() >>> frame, and then "print /x cpu->isar" ? That should show us >>> what we think we've got as ID registers from the kernel. >>> (You might need to build QEMU with --enable-debug to get >>> useful enough debug info to do that, not sure.) >> >> (My qemu build script always builds QEMU in two configs, the difference >> being --prefix and --enable-debug.) >> >> This is what I got: >> >> (gdb) frame 4 >> #4 0x00000000006a063c in arm_cpu_realizefn (dev=0x1761140, >> errp=0xffffffffe540) >> at .../qemu/target/arm/cpu.c:1159 >> 1159 assert(no_aa32 || cpu_isar_feature(arm_div, cpu)); >> (gdb) print /x cpu->isar >> $1 = {id_isar0 = 0x0, id_isar1 = 0x0, id_isar2 = 0x0, id_isar3 = 0x0, >> id_isar4 = 0x0, id_isar5 = 0x0, id_isar6 = 0x0, mvfr0 = 0x0, >> mvfr1 = 0x0, mvfr2 = 0x0, id_aa64isar0 = 0x0, id_aa64isar1 = 0x0, >> id_aa64pfr0 = 0x11, id_aa64pfr1 = 0x0, id_aa64mmfr0 = 0x0, >> id_aa64mmfr1 = 0x0} > > For ISAR0, DIVIDE=0 > > so cpu_isar_feature(arm_div, cpu)=false > > For AA64PFR0, EL0=1, EL1=1. > > EL0 = 1: EL0 can be executed in AArch64 state only. > EL1 = 1: EL1 can be executed in AArch64 state only. > > so cpu_isar_feature(aa64_aa32, cpu)=false > then no_aa32=true > > The commit description is "on a host that doesn't support aarch32 mode > at all, neither arm_div nor jazelle will be supported either." > > Shouldn't we use a slighly different logic? Such: > > - assert(no_aa32 || cpu_isar_feature(arm_div, cpu)); > + assert(no_aa32 && !cpu_isar_feature(arm_div, cpu)); >
I'm unsure. The current formula seems to match the commit description. Implication -- that is, "A implies B", (A-->B) -- is equivalent to (!A || B). We have "no_aa32 || arm_div", which corresponds to "aa32 implies arm_div" (aa32-->arm_div). And that seems to match exactly what Peter said. The assert you suggest would fire on a host that supports at least one of aa32 and arm_div (= the assertion would fail if (aa32 || arm_div)). That would break on my host (hw+kernel) just the same, in the end. To substitute the boolean values: - assert(false || false) + assert(false && true) Thanks Laszlo