Awkwardly, M-profile uses a different ID register field from
A-profile to indicate presence of 16-bit floating point arithmetic.
This patchset corrects the feature test function. In order to
use the correct test, we need to be able to ask "is this M-profile?"
in the isar_feature function, and we don't have the 'env' pointer
there so we can't use the existing ARM_FEATURE_M feature bit check.

So we have to test the ID_PFR1 MProgMode field, but to do that we
need to give Cortex-M0 (a 'baseline' M-profile CPU which doesn't have
guest-visible ID registers) some ID register values which correspond
to what it implements, but hide those values from the guest. We
also need to do the usual move of id_pfr* into ARMISARegisters.

Patch 1 is unrelated: it's just a simple removal of an ARM_FEATURE
bit that was easy to convert to an isar feature field check because
we only test the condition in two places.

This series leaves us with two different ways to check for "is this
M-profile?" -- the old ARM_FEATURE_M bit, and also the new
isar_feature_aa32_mprofile() test.  We could in theory convert the
users of ARM_FEATURE_M, but I haven't, because there are a lot of
callsites, and it can't be done automatically:

 * arm_feature() takes the CPUARMState* whereas cpu_isar_feature()
   takes the ARMCPU*, so each callsite needs manual attention to
   identify whether it already has a 'cpu' local variable and
   add one if it doesn't
 * awkwardly, the KVM-on-AArch64-only host case does not zero
   out these ID register fields (see the comment in kvm64.c
   kvm_arm_get_host_cpu_features()), so calling the
   isar_feature function is only valid if we already know that
   this CPU is AArch32 -- this would need manually checking
   at all callsites. (Unless we wanted to change our minds about
   leaving UNKNOWN values in the ID register fields.)

thanks
-- PMM

Peter Maydell (5):
  target/arm: Replace ARM_FEATURE_PXN with ID_MMFR0.VMSA check
  target/arm: Move id_pfr0, id_pfr1 into ARMISARegisters
  hw/intc/armv7m_nvic: Only show ID register values for Main Extension
    CPUs
  target/arm: Add ID register values for Cortex-M0
  target/arm: Make isar_feature_aa32_fp16_arith() handle M-profile

 target/arm/cpu.h      | 50 ++++++++++++++++++++++++++++++------
 hw/intc/armv7m_nvic.c | 46 +++++++++++++++++++++++++++++++--
 target/arm/cpu.c      | 21 ++++++++-------
 target/arm/cpu64.c    | 12 ++++-----
 target/arm/cpu_tcg.c  | 60 ++++++++++++++++++++++++++++++-------------
 target/arm/helper.c   |  9 ++++---
 target/arm/kvm64.c    |  4 +++
 7 files changed, 153 insertions(+), 49 deletions(-)

-- 
2.20.1

On 9/10/20 10:38 AM, Peter Maydell wrote:
> This series leaves us with two different ways to check for "is this
> M-profile?" -- the old ARM_FEATURE_M bit, and also the new
> isar_feature_aa32_mprofile() test.  We could in theory convert the
> users of ARM_FEATURE_M, but I haven't, because there are a lot of
> callsites, and it can't be done automatically:
> 
>  * arm_feature() takes the CPUARMState* whereas cpu_isar_feature()
>    takes the ARMCPU*, so each callsite needs manual attention to
>    identify whether it already has a 'cpu' local variable and
>    add one if it doesn't
>  * awkwardly, the KVM-on-AArch64-only host case does not zero
>    out these ID register fields (see the comment in kvm64.c
>    kvm_arm_get_host_cpu_features()), so calling the
>    isar_feature function is only valid if we already know that
>    this CPU is AArch32 -- this would need manually checking
>    at all callsites. (Unless we wanted to change our minds about
>    leaving UNKNOWN values in the ID register fields.)

That is why I had suggested moving env->features to cpu->isar.features, so that
it would be nearby.

Which would give us

static inline bool isar_feature(const ARMISARegisters *id,
                                int feature)
{
    return extract64(id->features, feature, 1);
}

static inline bool arm_feature(CPUARMState *env, int feature)
{
    return isar_feature(&env_archcpu(env)->isar, feature);
}

But what you have at present solves the current problem, where we know we're in
aa32 mode in translate.c.


r~