On Thu, 4 Jun 2020 at 17:26, Leif Lindholm <l...@nuviainc.com> wrote: > > On Thu, Jun 04, 2020 at 17:09:30 +0100, Peter Maydell wrote: > > On Thu, 4 Jun 2020 at 17:03, Leif Lindholm <l...@nuviainc.com> wrote: > > > But there's also things like: > > > - a57_initfn explicitly setting kvm_target, then only being called > > > from max_initfn for !kvm_enabled() > > > > Expected -- a KVM 'max' is nothing to do with a TCG 'max': > > * for KVM, -cpu max means "same as -cpu host" > > * for TCG, -cpu max means "start with an A57, then add in all the > > extra architectural features that have been added since then". > > Sure. But why are we setting the kvm_target at all for the > !kvm_enabled() case?
Because it happens to be set in the a57 initfn, and it's harmless if we're not using TCG. I feel like some of your take on this set of functions comes from thinking of max_initfn as in some sense the 'primary' function here, when it's the other way around : a57_initfn is the standard kind of CPU initfn (behaving in and written the same way as a53_initfn and a72_initfn), whereas max_initfn is an odd special case which happens for convenience-of-implementation to piggyback on the a57 implementation. > > kvm_target being set by a57_initfn is specifically for the case > > where a KVM user is using "-cpu cortex-a57". > > > > > - a57_initfn setting cpu->dtb_compatible to "arm,cortex-a57" > > > > What else would it set it to? > > Hmm, I had been hoping there was a generic v8a one - kvm64.c kind of > got my hopes up with "arm,arm-v8". Ah, that's the other way around -- yes, for 'max' we should be using a more generic value, not accidentally using 'cortex-a57'. (Linux doesn't in practice care IIRC, which is why this bug hasn't been noticed.) But for an actual cortex-a57 model we should report as arm,cortex-a57. https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/cpus.yaml is the official list of permitted strings, incidentally. > > > - a57 initfn setting cpu->midr, max_initfn overwriting parts of it > > > > Also expected, TCG's -cpu max is "A57 with lots of extras". > > Maybe I'm being too rigid in my thinking here, but it does kind of jar > to see code call a function called aarch64_a57_initfn to have it write > a value where it then throws away the only thing in the value that > made it a57. The things that make it an A57 are all the feature flag and ID register values (which is is what tells the TCG implementation how the CPU should behave). The MIDR value is the least interesting bit of the a57_initfn in some ways: it doesn't change the QEMU emulation's behaviour and mostly guests don't care what the value is except for purposes of installing errata workarounds that don't matter for QEMU: your average guest would work just as well with any MIDR... > Not entirely unrelated question: > Would you take added field definitions in target/arm/cpu.h for > features not yet emulated in QEMU but defined in released versions of > ARM ARM? Yes in theory (you'll see that we have a bunch of field definitions already which we don't yet implement, because we tend to define all the fields for an ID register at the point where we want to access one field), but on the other hand there's usually no need to actually use them unless we're adding the emulation for the feature, so the question is: what would you do with them if you added them? thanks -- PMM
