| Message ID | 20180915161738.25257-1-richard.henderson@linaro.org |
|---|---|
| Headers | show |
| Series | target/arm: Derive cpu id regs from features | expand |
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
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