Re: [Qemu-devel] [RFC PATCH 00/13] target/arm: Derive cpu id regs from features

[Peter Maydell]

On 19 September 2018 at 08:44, Alex Bennée <address@hidden> wrote:
>
> Richard Henderson <address@hidden> 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