[RFC,00/13] target/arm: Derive cpu id regs from features

Message ID 20180915161738.25257-1-richard.henderson@linaro.org
Headers show
Series
  • target/arm: Derive cpu id regs from features
Related show

Message

Richard Henderson Sept. 15, 2018, 4:17 p.m.
This is something we talked about in the context of enabling sve in
system mode.  We don't want to replicate info between these two locations.

I'm not 100% happy with this, thus the RFC.  In particular, there are
several places in id_isar0, id_isar2, and id_isar4 that expose micro-
architectural details of the cpus.  We cannot infer these values.
We'll not be able to replicate the exact id values without additional
changes.

But I'll also note that with ARM_FEATURE_SWP, we're now at 60 feature
bits, which means that we only have 4 remaining before we have to come
up with another solution there.

I do wonder if we should instead introduce some little inline functions
to test each of the current feature bits, and once that's done convert
those to test cpu->id_* bits.

Most, but not all, of the feature bits would go away.  We'd have the
exact id values one would expect for a given cpu without having to
replicate the info.

Thoughts, one way or the other?


r~


Richard Henderson (13):
  target/arm: Add ARM_FEATURE_SWP
  target/arm: Derive id_isar0 from features
  target/arm: Derive id_isar1 from features
  target/arm: Derive id_isar2 from features
  target/arm: Derive id_isar3 from features
  target/arm: Derive id_isar4 from features
  target/arm: Derive id_isar5 and id_isar6 from features
  target/arm: Derive id_pfr0 from features
  target/arm: Derive id_pfr1 from features
  target/arm: Derive id_aa64isar0 from features
  target/arm: Derive id_aa64isar1 from features
  target/arm: Derive id_aa64pfr0 from features
  target/arm: Remove assertions from resolve_id_regs

 target/arm/cpu.h       |   1 +
 linux-user/elfload.c   |   3 +-
 target/arm/cpu.c       | 381 +++++++++++++++++++++++++++++++++++++++++
 target/arm/translate.c |   4 +
 4 files changed, 388 insertions(+), 1 deletion(-)

-- 
2.17.1

Comments

Alex Bennée Sept. 19, 2018, 3:44 p.m. | #1
Richard Henderson <richard.henderson@linaro.org> writes:

> This is something we talked about in the context of enabling sve in

> system mode.  We don't want to replicate info between these two locations.

>

> I'm not 100% happy with this, thus the RFC.  In particular, there are

> several places in id_isar0, id_isar2, and id_isar4 that expose micro-

> architectural details of the cpus.  We cannot infer these values.

> We'll not be able to replicate the exact id values without additional

> changes.


Can you remind me of which patch it was in the SVE system mode series
that prompted this as patches 1-3 of that series seem perfectly
reasonable to me. Without seeing a case where this is manifestly better
than the alternative it feels a bit like too much churn for little
direct benefit. OTOH it doesn't make things worse (not withstanding
fixing the final assert failures).

> But I'll also note that with ARM_FEATURE_SWP, we're now at 60 feature

> bits, which means that we only have 4 remaining before we have to come

> up with another solution there.


Do we? I seem to recall Peter had played with widening the feature field
without any major issue. Given they are static checks I would have
thought the compiler would just end up using a slightly different offset
to test the higher bits.

>

> I do wonder if we should instead introduce some little inline functions

> to test each of the current feature bits, and once that's done convert

> those to test cpu->id_* bits.

>

> Most, but not all, of the feature bits would go away.  We'd have the

> exact id values one would expect for a given cpu without having to

> replicate the info.

>

> Thoughts, one way or the other?


I haven't looked too closely at the individual bit patterns as reviewing
system registers on a single screen laptop is a little painful compared
to at home. However you can at least have an:

Acked-by: Alex Bennée <alex.bennee@linaro.org>


for the series. I guess I'm ambivalent about this particular series.

>

>

> r~

>

>

> Richard Henderson (13):

>   target/arm: Add ARM_FEATURE_SWP

>   target/arm: Derive id_isar0 from features

>   target/arm: Derive id_isar1 from features

>   target/arm: Derive id_isar2 from features

>   target/arm: Derive id_isar3 from features

>   target/arm: Derive id_isar4 from features

>   target/arm: Derive id_isar5 and id_isar6 from features

>   target/arm: Derive id_pfr0 from features

>   target/arm: Derive id_pfr1 from features

>   target/arm: Derive id_aa64isar0 from features

>   target/arm: Derive id_aa64isar1 from features

>   target/arm: Derive id_aa64pfr0 from features

>   target/arm: Remove assertions from resolve_id_regs

>

>  target/arm/cpu.h       |   1 +

>  linux-user/elfload.c   |   3 +-

>  target/arm/cpu.c       | 381 +++++++++++++++++++++++++++++++++++++++++

>  target/arm/translate.c |   4 +

>  4 files changed, 388 insertions(+), 1 deletion(-)



--
Alex Bennée
Peter Maydell Sept. 19, 2018, 3:57 p.m. | #2
On 19 September 2018 at 08:44, Alex Bennée <alex.bennee@linaro.org> wrote:
>

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

>

>> This is something we talked about in the context of enabling sve in

>> system mode.  We don't want to replicate info between these two locations.

>>

>> I'm not 100% happy with this, thus the RFC.  In particular, there are

>> several places in id_isar0, id_isar2, and id_isar4 that expose micro-

>> architectural details of the cpus.  We cannot infer these values.

>> We'll not be able to replicate the exact id values without additional

>> changes.

>

> Can you remind me of which patch it was in the SVE system mode series

> that prompted this as patches 1-3 of that series seem perfectly

> reasonable to me. Without seeing a case where this is manifestly better

> than the alternative it feels a bit like too much churn for little

> direct benefit. OTOH it doesn't make things worse (not withstanding

> fixing the final assert failures).


The issue is that the SVE patchset approach leaves us with a mix of:
 * ID registers which are hard-coded values set in the CPU init fns
 * feature bit settings
 * ID register fields which get overridden based on feature bits

which is a bit of a mess. I am hoping we can get to a more
consistent setup, where what a CPU supports is indicated in
one place, not two, and we consistently either (a) specify
the ID register fields in the CPU init fns and use those to
drive what features we enable or (b) determine the ID fields
from the feature bits. i don't particularly want some mix of the
two depending on what feature this is.

(The patchset for adding better perf counter support also runs into this.)

I have no direct opinion on how we should straighten this out, but
I do have a meta-opinion that we should come up with a consistent
approach of some kind.

>> But I'll also note that with ARM_FEATURE_SWP, we're now at 60 feature

>> bits, which means that we only have 4 remaining before we have to come

>> up with another solution there.

>

> Do we? I seem to recall Peter had played with widening the feature field

> without any major issue. Given they are static checks I would have

> thought the compiler would just end up using a slightly different offset

> to test the higher bits.


Yes, since arm_feature() takes a number 0..63, not the 1 << n value,
we could easily make it look in an env->features[] array.

thanks
-- PMM