My previous patch set for replacing feature bits with id registers
failed to consider that these id registers are beginning to control
migration, and thus we must fill them in for KVM as well.

Thus, we want to initialize these values within CPU from the host.

Finally, re-send the T32EE conversion patch, fixing the build
failure on an arm32 host in kvm32.c.

Tested on arm64; cross-build tested for arm32.


r~


Richard Henderson (5):
  target/arm: Install ARMISARegisters from kvm host
  target/arm: Fill in ARMISARegisters for kvm64
  target/arm: Introduce read_sys_reg32 for kvm32
  target/arm: Fill in ARMISARegisters for kvm32
  target/arm: Convert t32ee from feature bit to isar3 test

 target/arm/cpu.h     |  6 +++-
 target/arm/kvm_arm.h |  1 +
 linux-user/elfload.c |  2 +-
 target/arm/cpu.c     |  4 ---
 target/arm/helper.c  |  2 +-
 target/arm/kvm.c     |  1 +
 target/arm/kvm32.c   | 75 +++++++++++++++++++++++++-------------------
 target/arm/kvm64.c   | 63 +++++++++++++++++++++++++++++++++++--
 target/arm/machine.c |  3 +-
 9 files changed, 114 insertions(+), 43 deletions(-)

-- 
2.17.2

Richard Henderson <richard.henderson@linaro.org> writes:

> My previous patch set for replacing feature bits with id registers
> failed to consider that these id registers are beginning to control
> migration, and thus we must fill them in for KVM as well.
>
> Thus, we want to initialize these values within CPU from the host.
>
> Finally, re-send the T32EE conversion patch, fixing the build
> failure on an arm32 host in kvm32.c.
>
> Tested on arm64; cross-build tested for arm32.

I'm still seeing the following assert on qemu-test:

  qemu-system-aarch64: /home/alex/lsrc/qemu.git/target/arm/cpu.c:832: arm_cpu_realizefn: Assertion `cpu_isar_feature(arm_div, cpu)' failed.

Which is a regression caused by:

  7e0cf8b: target/arm: Convert division from feature bits to isar0 tests

I think the problem is the we trip over the assert because:

    /* Some features automatically imply others: */
    if (arm_feature(env, ARM_FEATURE_V8)) {
        if (arm_feature(env, ARM_FEATURE_M)) {
            set_feature(env, ARM_FEATURE_V7);
        } else {
            set_feature(env, ARM_FEATURE_V7VE);
        }
    }

Allows:

    if (arm_feature(env, ARM_FEATURE_V7VE)) {
        assert(cpu_isar_feature(arm_div, cpu));

Which isn't strictly true on kvm guests.


>
>
> r~
>
>
> Richard Henderson (5):
>   target/arm: Install ARMISARegisters from kvm host
>   target/arm: Fill in ARMISARegisters for kvm64
>   target/arm: Introduce read_sys_reg32 for kvm32
>   target/arm: Fill in ARMISARegisters for kvm32
>   target/arm: Convert t32ee from feature bit to isar3 test
>
>  target/arm/cpu.h     |  6 +++-
>  target/arm/kvm_arm.h |  1 +
>  linux-user/elfload.c |  2 +-
>  target/arm/cpu.c     |  4 ---
>  target/arm/helper.c  |  2 +-
>  target/arm/kvm.c     |  1 +
>  target/arm/kvm32.c   | 75 +++++++++++++++++++++++++-------------------
>  target/arm/kvm64.c   | 63 +++++++++++++++++++++++++++++++++++--
>  target/arm/machine.c |  3 +-
>  9 files changed, 114 insertions(+), 43 deletions(-)


--
Alex Bennée

On 1 November 2018 at 17:26, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> My previous patch set for replacing feature bits with id registers
>> failed to consider that these id registers are beginning to control
>> migration, and thus we must fill them in for KVM as well.
>>
>> Thus, we want to initialize these values within CPU from the host.
>>
>> Finally, re-send the T32EE conversion patch, fixing the build
>> failure on an arm32 host in kvm32.c.
>>
>> Tested on arm64; cross-build tested for arm32.
>
> I'm still seeing the following assert on qemu-test:
>
>   qemu-system-aarch64: /home/alex/lsrc/qemu.git/target/arm/cpu.c:832: arm_cpu_realizefn: Assertion `cpu_isar_feature(arm_div, cpu)' failed.
>
> Which is a regression caused by:
>
>   7e0cf8b: target/arm: Convert division from feature bits to isar0 tests
>
> I think the problem is the we trip over the assert because:
>
>     /* Some features automatically imply others: */
>     if (arm_feature(env, ARM_FEATURE_V8)) {
>         if (arm_feature(env, ARM_FEATURE_M)) {
>             set_feature(env, ARM_FEATURE_V7);
>         } else {
>             set_feature(env, ARM_FEATURE_V7VE);
>         }
>     }
>
> Allows:
>
>     if (arm_feature(env, ARM_FEATURE_V7VE)) {
>         assert(cpu_isar_feature(arm_div, cpu));
>
> Which isn't strictly true on kvm guests.

KVM guests should definitely all be v7VE and all have
the arm divide instruction, if they implement AArch32 at all.
I think what we're hitting here is the case where the host
CPU has no AArch32 support. In that case the ID_ISAR0_EL1
sysreg (which we read from KVM and use to populate the
cpu->isar struct) has an UNKNOWN value.

thanks
-- PMM

On 1 November 2018 at 17:30, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 November 2018 at 17:26, Alex Bennée <alex.bennee@linaro.org> wrote:
>> I think the problem is the we trip over the assert because:
>>
>>     /* Some features automatically imply others: */
>>     if (arm_feature(env, ARM_FEATURE_V8)) {
>>         if (arm_feature(env, ARM_FEATURE_M)) {
>>             set_feature(env, ARM_FEATURE_V7);
>>         } else {
>>             set_feature(env, ARM_FEATURE_V7VE);
>>         }
>>     }
>>
>> Allows:
>>
>>     if (arm_feature(env, ARM_FEATURE_V7VE)) {
>>         assert(cpu_isar_feature(arm_div, cpu));
>>
>> Which isn't strictly true on kvm guests.
>
> KVM guests should definitely all be v7VE and all have
> the arm divide instruction, if they implement AArch32 at all.
> I think what we're hitting here is the case where the host
> CPU has no AArch32 support. In that case the ID_ISAR0_EL1
> sysreg (which we read from KVM and use to populate the
> cpu->isar struct) has an UNKNOWN value.

So I think the assert should probably be
"assert that if this cpu has any EL that can execute at
AArch32 then it has the arm_div ISAR feature". You can
determine the former by looking at ID_AA64PFR0_EL1 field
EL0 (bits [3:0]), which will be >= 0b0010 if AArch32 is
supported at any EL.

thanks
-- PMM