Re: [Qemu-devel] [PULL 08/10] target/arm: Conditionalize some asserts on aarch32 support

[Peter Maydell]

On Wed, 17 Jul 2019 at 10:22, Laszlo Ersek <address@hidden> wrote:
> Hmmm wait a second. The ARMv8 ARM says, about ID_ISAR0_EL1:
>
> > Divide, bits [27:24]
> >
> >     Indicates the implemented Divide instructions. Permitted values
> >     are:
> >     0000 None implemented.
> >     0001 Adds SDIV and UDIV in the T32 instruction set.
> >     0010 As for 0b0001, and adds SDIV and UDIV in the A32 instruction
> >          set.
> >     All other values are reserved.
>
> So this means that (aa32 && !arm_div) *does* conform to the architecture
> manual!

The architecture manual also says
"In ARMv8-A the only permitted value is 0010.", and in
earlier versions of the manual it says also that ARMv7VE
implies that we must have all the integer divide instructions.
And this assert is guarded by "if (arm_feature(env, ARM_FEATURE_V7VE))".
(This is what we're trying to test, really: something that's
providing AArch32 v7VE-or-better must include the divide insns.)

> Upon re-reading the commit message more carefully:
>
>     on a host that doesn't support aarch32 mode at all, neither arm_div
>     nor jazelle will be supported either

The point of this text is that "v8 AArch64 with no AArch32" is
an exception to the previous version of the check, which was
just "v7VE must mean we have the divide insns".

Similarly with Jazelle: what we were testing before commit 0f8d06f
was "v6 must imply Jazelle", which is true for everything except
the special case of "an AArch64 CPU with no AArch32 support" (which
can only happen for a KVM setup), and the test we are trying to
check after the commit is "v6 aarch32 must imply Jazelle".

> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index e75a64a25a4b..ea84a3e11abb 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1382,8 +1382,13 @@ static void arm_cpu_realizefn(DeviceState *dev, 
> > Error **errp)
> >           * include the various other features that V7VE implies.
> >           * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
> >           * Security Extensions is ARM_FEATURE_EL3.
> > +         *
> > +         * Lack of aa32 support excludes arm_div support:
> > +         *   no_aa32 --> !arm_div
> > +         * Using the logical OR operator, the same is expressed as:
> > +         *   !no_aa32 || !arm_div
> >           */
> > -        assert(no_aa32 || cpu_isar_feature(arm_div, cpu));
> > +        assert(!no_aa32 || !cpu_isar_feature(arm_div, cpu));

This now fails to test what we wanted, because it will allow
an incorrect set of ID registers that define a v7VE CPU
without the arm_div feature to pass.

thanks
-- PMM