* kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions @ 2020-06-04 12:55 Leif Lindholm 2020-06-04 13:10 ` Peter Maydell 2020-06-04 13:18 ` Andrew Jones 0 siblings, 2 replies; 18+ messages in thread From: Leif Lindholm @ 2020-06-04 12:55 UTC (permalink / raw) To: qemu-devel; +Cc: Peter Maydell, qemu-arm, Paolo Bonzini Hi there, (all this done on current HEAD: 66234fee9c) I was looking through the definition of the aarch64 "max" cpu, and noticed it invokes aarch64_a57_initfn as a template, followed by overriding some feature and ID fields to enable additional functionality. I then noticed it does not override kvm_target, which hence remains set as QEMU_KVM_ARM_TARGET_CORTEX_A57. Of course, this only happens on the else side of the if (kvm_enabled()) branch, so doesn't affect it. However, while looking at this, I noticed aarch64_a72_initfn doesn't initialise kvm_target at all. Then I looked at the definition of QEMU_KVM_ARM_TARGET_CORTEX_A57, found there was also a KVM_ARM_TARGET_CORTEX_A57, and then I noticed there exists a KVM_ARM_TARGET_CORTEX_A57 (in linux-headers/asm-arm64/kvm.h), and *then* I noticed there exists a KVM_ARM_TARGET_GENERIC_V8 definition there as well - plus a comment saying "please don't add any more targets unless you really need to". Then I noticed there isn't a corresponding QEMU_KVM_ARM_TARGET_GENERIC_V8 in target/arm/kvm-consts.h. So, then I decided to actually test things, and found that (with -enable-kvm): - on Cortex-A53 hardware - "max" kvm_target gets initialized to 4 (KVM_ARM_TARGET_CORTEX_A53) by kvm_arm_get_host_cpu_features (as returned from the kernel for vm_arm_create_scratch_host_vcpu) - cortex-A72 fails to start with "KVM is not supported for this guest CPU type" (fair enough, it's later than A53) - on Cortex-A72 hardware - "max" kvm_target gets initialized to 5 (KVM_ARM_TARGET_GENERIC_V8) by kvm_arm_get_host_cpu_features - "cortex-A72" fails to start (umm...) However ... if I haven't managed to confuse myself somewhere in here (which is completely possible), would it be OK if I submitted a set of patches that: - add a QEMU_KVM_ARM_TARGET_GENERIC_V8 to match the kernel one - set kvm_target for Cortex-A72 to QEMU_KVM_ARM_TARGET_GENERIC_V8 - alternatively drop the explicit settings for A57/A53 - drop the call from aarch64_max_initfn to aarch64_a57_initfn, and copy the relevant bits into the former for the !kvm case ? / Leif ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions 2020-06-04 12:55 kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions Leif Lindholm @ 2020-06-04 13:10 ` Peter Maydell 2020-06-04 13:18 ` Andrew Jones 1 sibling, 0 replies; 18+ messages in thread From: Peter Maydell @ 2020-06-04 13:10 UTC (permalink / raw) To: Leif Lindholm; +Cc: Paolo Bonzini, qemu-arm, QEMU Developers, kvmarm [added kvm-arm to the cc list; the kernel folks tend to hang out there, not on qemu-devel, so KVM related questions are usually worth raising there as well.] On Thu, 4 Jun 2020 at 13:55, Leif Lindholm <leif@nuviainc.com> wrote: > However, while looking at this, I noticed aarch64_a72_initfn doesn't > initialise kvm_target at all. Yep. The kernel doesn't support "give me a CPU that looks like a Cortex-A72". > So, then I decided to actually test things, and found that > (with -enable-kvm): > - on Cortex-A53 hardware > - "max" kvm_target gets initialized to 4 (KVM_ARM_TARGET_CORTEX_A53) > by kvm_arm_get_host_cpu_features (as returned from the kernel for > vm_arm_create_scratch_host_vcpu) > - cortex-A72 fails to start with "KVM is not supported for this guest > CPU type" > (fair enough, it's later than A53) Untested, but I assume that -cpu cortex-a53 works on the A53... > - on Cortex-A72 hardware > - "max" kvm_target gets initialized to 5 (KVM_ARM_TARGET_GENERIC_V8) > by kvm_arm_get_host_cpu_features > - "cortex-A72" fails to start (umm...) ...and fails on the A72 host. > However ... if I haven't managed to confuse myself somewhere in here > (which is completely possible), would it be OK if I submitted a set of > patches that: > - add a QEMU_KVM_ARM_TARGET_GENERIC_V8 to match the kernel one > - set kvm_target for Cortex-A72 to QEMU_KVM_ARM_TARGET_GENERIC_V8 This would be wrong -- it would mean that you could tell QEMU "give me a guest CPU that's a Cortex-A72" and it would not error on non-A72 hardware but not actually give a guest CPU that looks like a Cortex-A72. * If what you want is "give me something that works" then that's -cpu host or -cpu max. * If what you want is "give me something that's always this kind of CPU regardless of the host hardware" then that's a lot of kernel dev work nobody's been enthusiastic enough to undertake yet (notably the "what do we do about CPU errata workarounds" question would need to be solved...) * If what you want is "allow me to say '-cpu cortex-a72' and have it work on an A72 host and not anywhere else" then I guess we could implement that on the QEMU side by querying the MIDR and checking whether it was what we expected. > - alternatively drop the explicit settings for A57/A53 These explicit settings are correct, because for these CPUs the kernel does have a "give me what I want in particular" setting (which it will fail on the wrong h/w), and also as back-compat for older kernels that predate the GENERIC_V8 define and only recognize the explicit "give me an A53" value. > - drop the call from aarch64_max_initfn to aarch64_a57_initfn, and > copy the relevant bits into the former for the !kvm case Not sure why you care about this -- it's an implementation detail of the TCG handling of the 'max' CPU. There's an argument for disentangling TCG 'max' so it's not literally implemented as "a57 plus newer stuff", granted. thanks -- PMM ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions @ 2020-06-04 13:10 ` Peter Maydell 0 siblings, 0 replies; 18+ messages in thread From: Peter Maydell @ 2020-06-04 13:10 UTC (permalink / raw) To: Leif Lindholm; +Cc: Paolo Bonzini, qemu-arm, QEMU Developers, kvmarm [added kvm-arm to the cc list; the kernel folks tend to hang out there, not on qemu-devel, so KVM related questions are usually worth raising there as well.] On Thu, 4 Jun 2020 at 13:55, Leif Lindholm <leif@nuviainc.com> wrote: > However, while looking at this, I noticed aarch64_a72_initfn doesn't > initialise kvm_target at all. Yep. The kernel doesn't support "give me a CPU that looks like a Cortex-A72". > So, then I decided to actually test things, and found that > (with -enable-kvm): > - on Cortex-A53 hardware > - "max" kvm_target gets initialized to 4 (KVM_ARM_TARGET_CORTEX_A53) > by kvm_arm_get_host_cpu_features (as returned from the kernel for > vm_arm_create_scratch_host_vcpu) > - cortex-A72 fails to start with "KVM is not supported for this guest > CPU type" > (fair enough, it's later than A53) Untested, but I assume that -cpu cortex-a53 works on the A53... > - on Cortex-A72 hardware > - "max" kvm_target gets initialized to 5 (KVM_ARM_TARGET_GENERIC_V8) > by kvm_arm_get_host_cpu_features > - "cortex-A72" fails to start (umm...) ...and fails on the A72 host. > However ... if I haven't managed to confuse myself somewhere in here > (which is completely possible), would it be OK if I submitted a set of > patches that: > - add a QEMU_KVM_ARM_TARGET_GENERIC_V8 to match the kernel one > - set kvm_target for Cortex-A72 to QEMU_KVM_ARM_TARGET_GENERIC_V8 This would be wrong -- it would mean that you could tell QEMU "give me a guest CPU that's a Cortex-A72" and it would not error on non-A72 hardware but not actually give a guest CPU that looks like a Cortex-A72. * If what you want is "give me something that works" then that's -cpu host or -cpu max. * If what you want is "give me something that's always this kind of CPU regardless of the host hardware" then that's a lot of kernel dev work nobody's been enthusiastic enough to undertake yet (notably the "what do we do about CPU errata workarounds" question would need to be solved...) * If what you want is "allow me to say '-cpu cortex-a72' and have it work on an A72 host and not anywhere else" then I guess we could implement that on the QEMU side by querying the MIDR and checking whether it was what we expected. > - alternatively drop the explicit settings for A57/A53 These explicit settings are correct, because for these CPUs the kernel does have a "give me what I want in particular" setting (which it will fail on the wrong h/w), and also as back-compat for older kernels that predate the GENERIC_V8 define and only recognize the explicit "give me an A53" value. > - drop the call from aarch64_max_initfn to aarch64_a57_initfn, and > copy the relevant bits into the former for the !kvm case Not sure why you care about this -- it's an implementation detail of the TCG handling of the 'max' CPU. There's an argument for disentangling TCG 'max' so it's not literally implemented as "a57 plus newer stuff", granted. thanks -- PMM _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions 2020-06-04 13:10 ` Peter Maydell @ 2020-06-04 13:32 ` Andrew Jones -1 siblings, 0 replies; 18+ messages in thread From: Andrew Jones @ 2020-06-04 13:32 UTC (permalink / raw) To: Peter Maydell Cc: Paolo Bonzini, Leif Lindholm, QEMU Developers, qemu-arm, kvmarm On Thu, Jun 04, 2020 at 02:10:08PM +0100, Peter Maydell wrote: > [added kvm-arm to the cc list; the kernel folks tend to hang out > there, not on qemu-devel, so KVM related questions are usually > worth raising there as well.] > > On Thu, 4 Jun 2020 at 13:55, Leif Lindholm <leif@nuviainc.com> wrote: > > However, while looking at this, I noticed aarch64_a72_initfn doesn't > > initialise kvm_target at all. > > Yep. The kernel doesn't support "give me a CPU that looks like > a Cortex-A72". > > > So, then I decided to actually test things, and found that > > (with -enable-kvm): > > - on Cortex-A53 hardware > > - "max" kvm_target gets initialized to 4 (KVM_ARM_TARGET_CORTEX_A53) > > by kvm_arm_get_host_cpu_features (as returned from the kernel for > > vm_arm_create_scratch_host_vcpu) > > - cortex-A72 fails to start with "KVM is not supported for this guest > > CPU type" > > (fair enough, it's later than A53) > > Untested, but I assume that -cpu cortex-a53 works on the A53... > > > - on Cortex-A72 hardware > > - "max" kvm_target gets initialized to 5 (KVM_ARM_TARGET_GENERIC_V8) > > by kvm_arm_get_host_cpu_features > > - "cortex-A72" fails to start (umm...) > > ...and fails on the A72 host. > > > However ... if I haven't managed to confuse myself somewhere in here > > (which is completely possible), would it be OK if I submitted a set of > > patches that: > > - add a QEMU_KVM_ARM_TARGET_GENERIC_V8 to match the kernel one > > - set kvm_target for Cortex-A72 to QEMU_KVM_ARM_TARGET_GENERIC_V8 > > This would be wrong -- it would mean that you could tell QEMU "give > me a guest CPU that's a Cortex-A72" and it would not error on > non-A72 hardware but not actually give a guest CPU that looks > like a Cortex-A72. > > * If what you want is "give me something that works" then that's > -cpu host or -cpu max. > > * If what you want is "give me something that's always this kind of > CPU regardless of the host hardware" then that's a lot of kernel > dev work nobody's been enthusiastic enough to undertake yet > (notably the "what do we do about CPU errata workarounds" question > would need to be solved...) > > * If what you want is "allow me to say '-cpu cortex-a72' and have > it work on an A72 host and not anywhere else" then I guess we could > implement that on the QEMU side by querying the MIDR and checking > whether it was what we expected. It would match what we have for A53 and A57, so in that case it sounds reasonable. OTOH, it may cause headaches when we want to finally say that CPU models work, because it won't be clear to a user that just using '-cpu cortex-a72' will really work, or just work on an A72 hosts like before. We'll have that problem with A53 and A57 too, but maybe we shouldn't add any more. > > > - alternatively drop the explicit settings for A57/A53 > > These explicit settings are correct, because for these CPUs > the kernel does have a "give me what I want in particular" > setting (which it will fail on the wrong h/w), and also as > back-compat for older kernels that predate the GENERIC_V8 > define and only recognize the explicit "give me an A53" value. Actually, I think the failing for the wrong hardware is about all these older targets do. I didn't look real closely, but I think all targets produce the same result for the guest, which is to pass through the host ID registers. > > > - drop the call from aarch64_max_initfn to aarch64_a57_initfn, and > > copy the relevant bits into the former for the !kvm case > > Not sure why you care about this -- it's an implementation > detail of the TCG handling of the 'max' CPU. There's an argument > for disentangling TCG 'max' so it's not literally implemented > as "a57 plus newer stuff", granted. > > thanks > -- PMM > Thanks, drew ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions @ 2020-06-04 13:32 ` Andrew Jones 0 siblings, 0 replies; 18+ messages in thread From: Andrew Jones @ 2020-06-04 13:32 UTC (permalink / raw) To: Peter Maydell Cc: Paolo Bonzini, Leif Lindholm, QEMU Developers, qemu-arm, kvmarm On Thu, Jun 04, 2020 at 02:10:08PM +0100, Peter Maydell wrote: > [added kvm-arm to the cc list; the kernel folks tend to hang out > there, not on qemu-devel, so KVM related questions are usually > worth raising there as well.] > > On Thu, 4 Jun 2020 at 13:55, Leif Lindholm <leif@nuviainc.com> wrote: > > However, while looking at this, I noticed aarch64_a72_initfn doesn't > > initialise kvm_target at all. > > Yep. The kernel doesn't support "give me a CPU that looks like > a Cortex-A72". > > > So, then I decided to actually test things, and found that > > (with -enable-kvm): > > - on Cortex-A53 hardware > > - "max" kvm_target gets initialized to 4 (KVM_ARM_TARGET_CORTEX_A53) > > by kvm_arm_get_host_cpu_features (as returned from the kernel for > > vm_arm_create_scratch_host_vcpu) > > - cortex-A72 fails to start with "KVM is not supported for this guest > > CPU type" > > (fair enough, it's later than A53) > > Untested, but I assume that -cpu cortex-a53 works on the A53... > > > - on Cortex-A72 hardware > > - "max" kvm_target gets initialized to 5 (KVM_ARM_TARGET_GENERIC_V8) > > by kvm_arm_get_host_cpu_features > > - "cortex-A72" fails to start (umm...) > > ...and fails on the A72 host. > > > However ... if I haven't managed to confuse myself somewhere in here > > (which is completely possible), would it be OK if I submitted a set of > > patches that: > > - add a QEMU_KVM_ARM_TARGET_GENERIC_V8 to match the kernel one > > - set kvm_target for Cortex-A72 to QEMU_KVM_ARM_TARGET_GENERIC_V8 > > This would be wrong -- it would mean that you could tell QEMU "give > me a guest CPU that's a Cortex-A72" and it would not error on > non-A72 hardware but not actually give a guest CPU that looks > like a Cortex-A72. > > * If what you want is "give me something that works" then that's > -cpu host or -cpu max. > > * If what you want is "give me something that's always this kind of > CPU regardless of the host hardware" then that's a lot of kernel > dev work nobody's been enthusiastic enough to undertake yet > (notably the "what do we do about CPU errata workarounds" question > would need to be solved...) > > * If what you want is "allow me to say '-cpu cortex-a72' and have > it work on an A72 host and not anywhere else" then I guess we could > implement that on the QEMU side by querying the MIDR and checking > whether it was what we expected. It would match what we have for A53 and A57, so in that case it sounds reasonable. OTOH, it may cause headaches when we want to finally say that CPU models work, because it won't be clear to a user that just using '-cpu cortex-a72' will really work, or just work on an A72 hosts like before. We'll have that problem with A53 and A57 too, but maybe we shouldn't add any more. > > > - alternatively drop the explicit settings for A57/A53 > > These explicit settings are correct, because for these CPUs > the kernel does have a "give me what I want in particular" > setting (which it will fail on the wrong h/w), and also as > back-compat for older kernels that predate the GENERIC_V8 > define and only recognize the explicit "give me an A53" value. Actually, I think the failing for the wrong hardware is about all these older targets do. I didn't look real closely, but I think all targets produce the same result for the guest, which is to pass through the host ID registers. > > > - drop the call from aarch64_max_initfn to aarch64_a57_initfn, and > > copy the relevant bits into the former for the !kvm case > > Not sure why you care about this -- it's an implementation > detail of the TCG handling of the 'max' CPU. There's an argument > for disentangling TCG 'max' so it's not literally implemented > as "a57 plus newer stuff", granted. > > thanks > -- PMM > Thanks, drew _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions 2020-06-04 13:32 ` Andrew Jones @ 2020-06-04 13:37 ` Peter Maydell -1 siblings, 0 replies; 18+ messages in thread From: Peter Maydell @ 2020-06-04 13:37 UTC (permalink / raw) To: Andrew Jones Cc: Paolo Bonzini, Leif Lindholm, QEMU Developers, qemu-arm, kvmarm On Thu, 4 Jun 2020 at 14:32, Andrew Jones <drjones@redhat.com> wrote: > On Thu, Jun 04, 2020 at 02:10:08PM +0100, Peter Maydell wrote: > > These explicit settings are correct, because for these CPUs > > the kernel does have a "give me what I want in particular" > > setting (which it will fail on the wrong h/w), and also as > > back-compat for older kernels that predate the GENERIC_V8 > > define and only recognize the explicit "give me an A53" value. > > Actually, I think the failing for the wrong hardware is about all these > older targets do. I didn't look real closely, but I think all targets > produce the same result for the guest, which is to pass through the host > ID registers. Yes; it's just that originally "specify CPU exactly" was the only interface, and there wasn't a GENERIC_V8 at all. I actually suspect that current QEMU will no longer work on a kernel that's so old that it lacks the GENERIC_V8 and PREFERRED_TARGET support[*], but we don't have an explicit "we need at least host kernel version X" requirement that we track, so it's hard to say for certain. (If we cared enough to test we could likely delete a bit of back-compat handling code in QEMU.) [*] in particular I have a feeling that recent changes to the GIC handling code in the virt board implicitly dropped handling for ancient kernels thanks -- PMM ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions @ 2020-06-04 13:37 ` Peter Maydell 0 siblings, 0 replies; 18+ messages in thread From: Peter Maydell @ 2020-06-04 13:37 UTC (permalink / raw) To: Andrew Jones Cc: Paolo Bonzini, Leif Lindholm, QEMU Developers, qemu-arm, kvmarm On Thu, 4 Jun 2020 at 14:32, Andrew Jones <drjones@redhat.com> wrote: > On Thu, Jun 04, 2020 at 02:10:08PM +0100, Peter Maydell wrote: > > These explicit settings are correct, because for these CPUs > > the kernel does have a "give me what I want in particular" > > setting (which it will fail on the wrong h/w), and also as > > back-compat for older kernels that predate the GENERIC_V8 > > define and only recognize the explicit "give me an A53" value. > > Actually, I think the failing for the wrong hardware is about all these > older targets do. I didn't look real closely, but I think all targets > produce the same result for the guest, which is to pass through the host > ID registers. Yes; it's just that originally "specify CPU exactly" was the only interface, and there wasn't a GENERIC_V8 at all. I actually suspect that current QEMU will no longer work on a kernel that's so old that it lacks the GENERIC_V8 and PREFERRED_TARGET support[*], but we don't have an explicit "we need at least host kernel version X" requirement that we track, so it's hard to say for certain. (If we cared enough to test we could likely delete a bit of back-compat handling code in QEMU.) [*] in particular I have a feeling that recent changes to the GIC handling code in the virt board implicitly dropped handling for ancient kernels thanks -- PMM _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions 2020-06-04 13:10 ` Peter Maydell @ 2020-06-04 15:38 ` Leif Lindholm -1 siblings, 0 replies; 18+ messages in thread From: Leif Lindholm @ 2020-06-04 15:38 UTC (permalink / raw) To: Peter Maydell; +Cc: Paolo Bonzini, qemu-arm, QEMU Developers, kvmarm On Thu, Jun 04, 2020 at 14:10:08 +0100, Peter Maydell wrote: > [added kvm-arm to the cc list; the kernel folks tend to hang out > there, not on qemu-devel, so KVM related questions are usually > worth raising there as well.] > > On Thu, 4 Jun 2020 at 13:55, Leif Lindholm <leif@nuviainc.com> wrote: > > However, while looking at this, I noticed aarch64_a72_initfn doesn't > > initialise kvm_target at all. > > Yep. The kernel doesn't support "give me a CPU that looks like > a Cortex-A72". > > > So, then I decided to actually test things, and found that > > (with -enable-kvm): > > - on Cortex-A53 hardware > > - "max" kvm_target gets initialized to 4 (KVM_ARM_TARGET_CORTEX_A53) > > by kvm_arm_get_host_cpu_features (as returned from the kernel for > > vm_arm_create_scratch_host_vcpu) > > - cortex-A72 fails to start with "KVM is not supported for this guest > > CPU type" > > (fair enough, it's later than A53) > > Untested, but I assume that -cpu cortex-a53 works on the A53... Yes. > > - on Cortex-A72 hardware > > - "max" kvm_target gets initialized to 5 (KVM_ARM_TARGET_GENERIC_V8) > > by kvm_arm_get_host_cpu_features > > - "cortex-A72" fails to start (umm...) > > ...and fails on the A72 host. From an explicit software test. If I initialize kvm_target to KVM_ARM_TARGET_GENERIC_V8, I can certainly run EDK2. > > However ... if I haven't managed to confuse myself somewhere in here > > (which is completely possible), would it be OK if I submitted a set of > > patches that: > > - add a QEMU_KVM_ARM_TARGET_GENERIC_V8 to match the kernel one > > - set kvm_target for Cortex-A72 to QEMU_KVM_ARM_TARGET_GENERIC_V8 > > This would be wrong -- it would mean that you could tell QEMU "give > me a guest CPU that's a Cortex-A72" and it would not error on > non-A72 hardware but not actually give a guest CPU that looks > like a Cortex-A72. > > * If what you want is "give me something that works" then that's > -cpu host or -cpu max. That's what I thought until I saw the manual A57/A53 setting of kvm_target. > * If what you want is "give me something that's always this kind of > CPU regardless of the host hardware" then that's a lot of kernel > dev work nobody's been enthusiastic enough to undertake yet > (notably the "what do we do about CPU errata workarounds" question > would need to be solved...) > > * If what you want is "allow me to say '-cpu cortex-a72' and have > it work on an A72 host and not anywhere else" then I guess we could > implement that on the QEMU side by querying the MIDR and checking > whether it was what we expected. I don't really. > > - alternatively drop the explicit settings for A57/A53 > > These explicit settings are correct, because for these CPUs > the kernel does have a "give me what I want in particular" > setting (which it will fail on the wrong h/w), and also as > back-compat for older kernels that predate the GENERIC_V8 > define and only recognize the explicit "give me an A53" value. Right. But then I got somewhat confused also by how https://git.qemu.org/?p=qemu.git;a=blob;f=target/arm/kvm64.c#l494 doesn't explicitly list KVM_ARM_TARGET_CORTEX_A53. > > - drop the call from aarch64_max_initfn to aarch64_a57_initfn, and > > copy the relevant bits into the former for the !kvm case > > Not sure why you care about this -- it's an implementation > detail of the TCG handling of the 'max' CPU. There's an argument > for disentangling TCG 'max' so it's not literally implemented > as "a57 plus newer stuff", granted. So ... the reason I care is because I'm adding a new cpu in my local branch, figured cpu64.c was a good starting point, and then followed a long string of repeating the questions "why?" and "why not?" while trying to understand why things were set up the way they are. And I ended up in a state where it looks like we do some things for A57 that we don't do for A53, and we do even fewer things for A72, but then we do (end up doing) all of the A57 bits again for max (for TCG only), and then overwriting them. Then I tried to build some sort of consistent working model in my head and send some questions off to the list rather than try to send out patches straight away as I figured the likelihood was high I had missed or misunderstood something. I guess what I'm really asking is if there is some legacy in here that can be cleaned up to make the expected behaviour for a new CPU more clear from looking at available code? And if there are specific legacy things that need to be kept around for compatibility that should not be implemented by new CPUs, if they could have some nice warnings attached. Regards, Leif ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions @ 2020-06-04 15:38 ` Leif Lindholm 0 siblings, 0 replies; 18+ messages in thread From: Leif Lindholm @ 2020-06-04 15:38 UTC (permalink / raw) To: Peter Maydell; +Cc: Paolo Bonzini, qemu-arm, QEMU Developers, kvmarm On Thu, Jun 04, 2020 at 14:10:08 +0100, Peter Maydell wrote: > [added kvm-arm to the cc list; the kernel folks tend to hang out > there, not on qemu-devel, so KVM related questions are usually > worth raising there as well.] > > On Thu, 4 Jun 2020 at 13:55, Leif Lindholm <leif@nuviainc.com> wrote: > > However, while looking at this, I noticed aarch64_a72_initfn doesn't > > initialise kvm_target at all. > > Yep. The kernel doesn't support "give me a CPU that looks like > a Cortex-A72". > > > So, then I decided to actually test things, and found that > > (with -enable-kvm): > > - on Cortex-A53 hardware > > - "max" kvm_target gets initialized to 4 (KVM_ARM_TARGET_CORTEX_A53) > > by kvm_arm_get_host_cpu_features (as returned from the kernel for > > vm_arm_create_scratch_host_vcpu) > > - cortex-A72 fails to start with "KVM is not supported for this guest > > CPU type" > > (fair enough, it's later than A53) > > Untested, but I assume that -cpu cortex-a53 works on the A53... Yes. > > - on Cortex-A72 hardware > > - "max" kvm_target gets initialized to 5 (KVM_ARM_TARGET_GENERIC_V8) > > by kvm_arm_get_host_cpu_features > > - "cortex-A72" fails to start (umm...) > > ...and fails on the A72 host. From an explicit software test. If I initialize kvm_target to KVM_ARM_TARGET_GENERIC_V8, I can certainly run EDK2. > > However ... if I haven't managed to confuse myself somewhere in here > > (which is completely possible), would it be OK if I submitted a set of > > patches that: > > - add a QEMU_KVM_ARM_TARGET_GENERIC_V8 to match the kernel one > > - set kvm_target for Cortex-A72 to QEMU_KVM_ARM_TARGET_GENERIC_V8 > > This would be wrong -- it would mean that you could tell QEMU "give > me a guest CPU that's a Cortex-A72" and it would not error on > non-A72 hardware but not actually give a guest CPU that looks > like a Cortex-A72. > > * If what you want is "give me something that works" then that's > -cpu host or -cpu max. That's what I thought until I saw the manual A57/A53 setting of kvm_target. > * If what you want is "give me something that's always this kind of > CPU regardless of the host hardware" then that's a lot of kernel > dev work nobody's been enthusiastic enough to undertake yet > (notably the "what do we do about CPU errata workarounds" question > would need to be solved...) > > * If what you want is "allow me to say '-cpu cortex-a72' and have > it work on an A72 host and not anywhere else" then I guess we could > implement that on the QEMU side by querying the MIDR and checking > whether it was what we expected. I don't really. > > - alternatively drop the explicit settings for A57/A53 > > These explicit settings are correct, because for these CPUs > the kernel does have a "give me what I want in particular" > setting (which it will fail on the wrong h/w), and also as > back-compat for older kernels that predate the GENERIC_V8 > define and only recognize the explicit "give me an A53" value. Right. But then I got somewhat confused also by how https://git.qemu.org/?p=qemu.git;a=blob;f=target/arm/kvm64.c#l494 doesn't explicitly list KVM_ARM_TARGET_CORTEX_A53. > > - drop the call from aarch64_max_initfn to aarch64_a57_initfn, and > > copy the relevant bits into the former for the !kvm case > > Not sure why you care about this -- it's an implementation > detail of the TCG handling of the 'max' CPU. There's an argument > for disentangling TCG 'max' so it's not literally implemented > as "a57 plus newer stuff", granted. So ... the reason I care is because I'm adding a new cpu in my local branch, figured cpu64.c was a good starting point, and then followed a long string of repeating the questions "why?" and "why not?" while trying to understand why things were set up the way they are. And I ended up in a state where it looks like we do some things for A57 that we don't do for A53, and we do even fewer things for A72, but then we do (end up doing) all of the A57 bits again for max (for TCG only), and then overwriting them. Then I tried to build some sort of consistent working model in my head and send some questions off to the list rather than try to send out patches straight away as I figured the likelihood was high I had missed or misunderstood something. I guess what I'm really asking is if there is some legacy in here that can be cleaned up to make the expected behaviour for a new CPU more clear from looking at available code? And if there are specific legacy things that need to be kept around for compatibility that should not be implemented by new CPUs, if they could have some nice warnings attached. Regards, Leif _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions 2020-06-04 15:38 ` Leif Lindholm @ 2020-06-04 15:59 ` Peter Maydell -1 siblings, 0 replies; 18+ messages in thread From: Peter Maydell @ 2020-06-04 15:59 UTC (permalink / raw) To: Leif Lindholm; +Cc: Paolo Bonzini, qemu-arm, QEMU Developers, kvmarm On Thu, 4 Jun 2020 at 16:38, Leif Lindholm <leif@nuviainc.com> wrote: > On Thu, Jun 04, 2020 at 14:10:08 +0100, Peter Maydell wrote: > > On Thu, 4 Jun 2020 at 13:55, Leif Lindholm <leif@nuviainc.com> wrote: > > > So, then I decided to actually test things, and found that > > > (with -enable-kvm): > > > - on Cortex-A53 hardware > > > - "max" kvm_target gets initialized to 4 (KVM_ARM_TARGET_CORTEX_A53) > > > by kvm_arm_get_host_cpu_features (as returned from the kernel for > > > vm_arm_create_scratch_host_vcpu) > > > - cortex-A72 fails to start with "KVM is not supported for this guest > > > CPU type" > > > (fair enough, it's later than A53) > > > > Untested, but I assume that -cpu cortex-a53 works on the A53... > > Yes. > > > > - on Cortex-A72 hardware > > > - "max" kvm_target gets initialized to 5 (KVM_ARM_TARGET_GENERIC_V8) > > > by kvm_arm_get_host_cpu_features > > > - "cortex-A72" fails to start (umm...) > > > > ...and fails on the A72 host. > > From an explicit software test. If I initialize kvm_target to > KVM_ARM_TARGET_GENERIC_V8, I can certainly run EDK2. That would be asking for a -cpu max, though, not for an A53. > > > However ... if I haven't managed to confuse myself somewhere in here > > > (which is completely possible), would it be OK if I submitted a set of > > > patches that: > > > - add a QEMU_KVM_ARM_TARGET_GENERIC_V8 to match the kernel one > > > - set kvm_target for Cortex-A72 to QEMU_KVM_ARM_TARGET_GENERIC_V8 > > > > This would be wrong -- it would mean that you could tell QEMU "give > > me a guest CPU that's a Cortex-A72" and it would not error on > > non-A72 hardware but not actually give a guest CPU that looks > > like a Cortex-A72. > > > > * If what you want is "give me something that works" then that's > > -cpu host or -cpu max. > > That's what I thought until I saw the manual A57/A53 setting of > kvm_target. > > > - alternatively drop the explicit settings for A57/A53 > > > > These explicit settings are correct, because for these CPUs > > the kernel does have a "give me what I want in particular" > > setting (which it will fail on the wrong h/w), and also as > > back-compat for older kernels that predate the GENERIC_V8 > > define and only recognize the explicit "give me an A53" value. > > Right. But then I got somewhat confused also by how > https://git.qemu.org/?p=qemu.git;a=blob;f=target/arm/kvm64.c#l494 > doesn't explicitly list KVM_ARM_TARGET_CORTEX_A53. That list is supposed to contain "all CPUs which might be present on a host kernel which doesn't support the PREFERRED_TARGET ioctl". That ioctl went in in Linux kernel commit 42c4e0c77ac91, and in a kernel source tree "git show 42c4e0c77ac91:arch/arm64/include/uapi/asm/kvm.h" tells us that indeed at that point the only 3 CPUs supported were AEM_V8, FOUNDATION_V8 and CORTEX_A57. Once the host kernel supported the PREFERRED_TARGET ioctl, we could query it to ask "what kind of CPU are you?" in that function rather than having to guess. > So ... the reason I care is because I'm adding a new cpu in my local > branch, figured cpu64.c was a good starting point, and then followed a > long string of repeating the questions "why?" and "why not?" while > trying to understand why things were set up the way they are. > > And I ended up in a state where it looks like we do some things for > A57 that we don't do for A53, and we do even fewer things for A72, but > then we do (end up doing) all of the A57 bits again for max (for TCG > only), and then overwriting them. (we don't overwrite them, we augment them.) > Then I tried to build some sort of consistent working model in my head > and send some questions off to the list rather than try to send out > patches straight away as I figured the likelihood was high I had > missed or misunderstood something. > > I guess what I'm really asking is if there is some legacy in here > that can be cleaned up to make the expected behaviour for a new CPU > more clear from looking at available code? And if there are specific > legacy things that need to be kept around for compatibility that > should not be implemented by new CPUs, if they could have some nice > warnings attached. At this point I'd take another step backwards and ask "why are you trying to add a new CPU?". Are you after a TCG emulation of it, or are you trying to use KVM? If you're using KVM, generally QEMU is set up so you don't need to tell it about new CPU types at all. Mostly the cpu definitions for specific CPUs in cpu64.c are there because we want them for TCG. thanks -- PMM ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions @ 2020-06-04 15:59 ` Peter Maydell 0 siblings, 0 replies; 18+ messages in thread From: Peter Maydell @ 2020-06-04 15:59 UTC (permalink / raw) To: Leif Lindholm; +Cc: Paolo Bonzini, qemu-arm, QEMU Developers, kvmarm On Thu, 4 Jun 2020 at 16:38, Leif Lindholm <leif@nuviainc.com> wrote: > On Thu, Jun 04, 2020 at 14:10:08 +0100, Peter Maydell wrote: > > On Thu, 4 Jun 2020 at 13:55, Leif Lindholm <leif@nuviainc.com> wrote: > > > So, then I decided to actually test things, and found that > > > (with -enable-kvm): > > > - on Cortex-A53 hardware > > > - "max" kvm_target gets initialized to 4 (KVM_ARM_TARGET_CORTEX_A53) > > > by kvm_arm_get_host_cpu_features (as returned from the kernel for > > > vm_arm_create_scratch_host_vcpu) > > > - cortex-A72 fails to start with "KVM is not supported for this guest > > > CPU type" > > > (fair enough, it's later than A53) > > > > Untested, but I assume that -cpu cortex-a53 works on the A53... > > Yes. > > > > - on Cortex-A72 hardware > > > - "max" kvm_target gets initialized to 5 (KVM_ARM_TARGET_GENERIC_V8) > > > by kvm_arm_get_host_cpu_features > > > - "cortex-A72" fails to start (umm...) > > > > ...and fails on the A72 host. > > From an explicit software test. If I initialize kvm_target to > KVM_ARM_TARGET_GENERIC_V8, I can certainly run EDK2. That would be asking for a -cpu max, though, not for an A53. > > > However ... if I haven't managed to confuse myself somewhere in here > > > (which is completely possible), would it be OK if I submitted a set of > > > patches that: > > > - add a QEMU_KVM_ARM_TARGET_GENERIC_V8 to match the kernel one > > > - set kvm_target for Cortex-A72 to QEMU_KVM_ARM_TARGET_GENERIC_V8 > > > > This would be wrong -- it would mean that you could tell QEMU "give > > me a guest CPU that's a Cortex-A72" and it would not error on > > non-A72 hardware but not actually give a guest CPU that looks > > like a Cortex-A72. > > > > * If what you want is "give me something that works" then that's > > -cpu host or -cpu max. > > That's what I thought until I saw the manual A57/A53 setting of > kvm_target. > > > - alternatively drop the explicit settings for A57/A53 > > > > These explicit settings are correct, because for these CPUs > > the kernel does have a "give me what I want in particular" > > setting (which it will fail on the wrong h/w), and also as > > back-compat for older kernels that predate the GENERIC_V8 > > define and only recognize the explicit "give me an A53" value. > > Right. But then I got somewhat confused also by how > https://git.qemu.org/?p=qemu.git;a=blob;f=target/arm/kvm64.c#l494 > doesn't explicitly list KVM_ARM_TARGET_CORTEX_A53. That list is supposed to contain "all CPUs which might be present on a host kernel which doesn't support the PREFERRED_TARGET ioctl". That ioctl went in in Linux kernel commit 42c4e0c77ac91, and in a kernel source tree "git show 42c4e0c77ac91:arch/arm64/include/uapi/asm/kvm.h" tells us that indeed at that point the only 3 CPUs supported were AEM_V8, FOUNDATION_V8 and CORTEX_A57. Once the host kernel supported the PREFERRED_TARGET ioctl, we could query it to ask "what kind of CPU are you?" in that function rather than having to guess. > So ... the reason I care is because I'm adding a new cpu in my local > branch, figured cpu64.c was a good starting point, and then followed a > long string of repeating the questions "why?" and "why not?" while > trying to understand why things were set up the way they are. > > And I ended up in a state where it looks like we do some things for > A57 that we don't do for A53, and we do even fewer things for A72, but > then we do (end up doing) all of the A57 bits again for max (for TCG > only), and then overwriting them. (we don't overwrite them, we augment them.) > Then I tried to build some sort of consistent working model in my head > and send some questions off to the list rather than try to send out > patches straight away as I figured the likelihood was high I had > missed or misunderstood something. > > I guess what I'm really asking is if there is some legacy in here > that can be cleaned up to make the expected behaviour for a new CPU > more clear from looking at available code? And if there are specific > legacy things that need to be kept around for compatibility that > should not be implemented by new CPUs, if they could have some nice > warnings attached. At this point I'd take another step backwards and ask "why are you trying to add a new CPU?". Are you after a TCG emulation of it, or are you trying to use KVM? If you're using KVM, generally QEMU is set up so you don't need to tell it about new CPU types at all. Mostly the cpu definitions for specific CPUs in cpu64.c are there because we want them for TCG. thanks -- PMM _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions 2020-06-04 12:55 kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions Leif Lindholm 2020-06-04 13:10 ` Peter Maydell @ 2020-06-04 13:18 ` Andrew Jones 2020-06-04 16:03 ` Leif Lindholm 1 sibling, 1 reply; 18+ messages in thread From: Andrew Jones @ 2020-06-04 13:18 UTC (permalink / raw) To: Leif Lindholm; +Cc: Peter Maydell, qemu-arm, qemu-devel, Paolo Bonzini On Thu, Jun 04, 2020 at 01:55:44PM +0100, Leif Lindholm wrote: > Hi there, > > (all this done on current HEAD: 66234fee9c) > > I was looking through the definition of the aarch64 "max" cpu, and > noticed it invokes aarch64_a57_initfn as a template, followed by > overriding some feature and ID fields to enable additional > functionality. > > I then noticed it does not override kvm_target, which hence remains > set as QEMU_KVM_ARM_TARGET_CORTEX_A57. Of course, this only happens on > the else side of the if (kvm_enabled()) branch, so doesn't affect it. > > However, while looking at this, I noticed aarch64_a72_initfn doesn't > initialise kvm_target at all. > > Then I looked at the definition of QEMU_KVM_ARM_TARGET_CORTEX_A57, > found there was also a KVM_ARM_TARGET_CORTEX_A57, and then I noticed > there exists a KVM_ARM_TARGET_CORTEX_A57 (in > linux-headers/asm-arm64/kvm.h), and *then* I noticed there exists a > KVM_ARM_TARGET_GENERIC_V8 definition there as well - plus a comment > saying "please don't add any more targets unless you really need to". > Then I noticed there isn't a corresponding > QEMU_KVM_ARM_TARGET_GENERIC_V8 in target/arm/kvm-consts.h. > > So, then I decided to actually test things, and found that > (with -enable-kvm): > - on Cortex-A53 hardware > - "max" kvm_target gets initialized to 4 (KVM_ARM_TARGET_CORTEX_A53) > by kvm_arm_get_host_cpu_features (as returned from the kernel for > vm_arm_create_scratch_host_vcpu) > - cortex-A72 fails to start with "KVM is not supported for this guest > CPU type" > (fair enough, it's later than A53) > - on Cortex-A72 hardware > - "max" kvm_target gets initialized to 5 (KVM_ARM_TARGET_GENERIC_V8) > by kvm_arm_get_host_cpu_features > - "cortex-A72" fails to start (umm...) > > However ... if I haven't managed to confuse myself somewhere in here > (which is completely possible), would it be OK if I submitted a set of > patches that: > - add a QEMU_KVM_ARM_TARGET_GENERIC_V8 to match the kernel one > - set kvm_target for Cortex-A72 to QEMU_KVM_ARM_TARGET_GENERIC_V8 I'd rather not do that. AArch64 KVM doesn't support CPU models, so we shouldn't try to fake it by allowing '-cpu cortex-a72' to work, even when using it on a host with Cortex-A72 CPUs. I'd rather all AArch64 KVM users use 'host' or 'max' (which is the same as 'host' when used with KVM) until CPU models truly work. > - alternatively drop the explicit settings for A57/A53 The A57 and A53 KVM targets would have to be exactly the same as the KVM generic target in order to do that. And, even then we can't remove them from KVM, as they're API, so it doesn't help much to change them in QEMU. > - drop the call from aarch64_max_initfn to aarch64_a57_initfn, and > copy the relevant bits into the former for the !kvm case I don't have a strong preference here, but if the naming is what's troublesome, then I'd think we're better off creating something like an aarch64_aXX_initfn() function and then calling it from both a57 and max, and anywhere else it fits. Thanks, drew ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions 2020-06-04 13:18 ` Andrew Jones @ 2020-06-04 16:03 ` Leif Lindholm 2020-06-04 16:09 ` Peter Maydell 0 siblings, 1 reply; 18+ messages in thread From: Leif Lindholm @ 2020-06-04 16:03 UTC (permalink / raw) To: Andrew Jones; +Cc: Peter Maydell, qemu-arm, qemu-devel, Paolo Bonzini On Thu, Jun 04, 2020 at 15:18:02 +0200, Andrew Jones wrote: > > - drop the call from aarch64_max_initfn to aarch64_a57_initfn, and > > copy the relevant bits into the former for the !kvm case > > I don't have a strong preference here, but if the naming is what's > troublesome, then I'd think we're better off creating something like > an aarch64_aXX_initfn() function and then calling it from both a57 > and max, and anywhere else it fits. Well, the naming isn't the only issue, although there looks like a certain amount of duplication could be deleted from a57/a53/a72 and also be used for max if there was a common initfn. But there's also things like: - a57_initfn explicitly setting kvm_target, then only being called from max_initfn for !kvm_enabled() - a57_initfn setting cpu->dtb_compatible to "arm,cortex-a57" - a57 initfn setting cpu->midr, max_initfn overwriting parts of it Best Regards, Leif ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions 2020-06-04 16:03 ` Leif Lindholm @ 2020-06-04 16:09 ` Peter Maydell 2020-06-04 16:26 ` Leif Lindholm 0 siblings, 1 reply; 18+ messages in thread From: Peter Maydell @ 2020-06-04 16:09 UTC (permalink / raw) To: Leif Lindholm; +Cc: Paolo Bonzini, Andrew Jones, qemu-arm, QEMU Developers On Thu, 4 Jun 2020 at 17:03, Leif Lindholm <leif@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". 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? > - a57 initfn setting cpu->midr, max_initfn overwriting parts of it Also expected, TCG's -cpu max is "A57 with lots of extras". The way we create a TCG -cpu max is a bit odd, as the code was originally written in a situation where A57 was the most advanced TCG CPU we had and there were no extra architectural features supported by our CPU emulation. Today we have an A72 model as well and a lot of extra architectural features, so the "code borrowed" to "extras added" ratio looks a bit unbalanced. Cleaning it up would not be a bad idea. thanks -- PMM ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions 2020-06-04 16:09 ` Peter Maydell @ 2020-06-04 16:26 ` Leif Lindholm 2020-06-04 18:43 ` Peter Maydell 0 siblings, 1 reply; 18+ messages in thread From: Leif Lindholm @ 2020-06-04 16:26 UTC (permalink / raw) To: Peter Maydell; +Cc: Paolo Bonzini, Andrew Jones, qemu-arm, QEMU Developers On Thu, Jun 04, 2020 at 17:09:30 +0100, Peter Maydell wrote: > On Thu, 4 Jun 2020 at 17:03, Leif Lindholm <leif@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? > 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". Still, for "max", would it not be useful to update it to the track the most architecturally advanced cpu supported? At this point "arm,cortex-a72". > > - 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 way we create a TCG -cpu max is a bit odd, as the code was > originally written in a situation where A57 was the most advanced > TCG CPU we had and there were no extra architectural features > supported by our CPU emulation. Today we have an A72 model as > well and a lot of extra architectural features, so the "code > borrowed" to "extras added" ratio looks a bit unbalanced. > Cleaning it up would not be a bad idea. I might start by doing that bit. It might make a lot of the above niggles simply disappear. 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? Regards, Leif ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions 2020-06-04 16:26 ` Leif Lindholm @ 2020-06-04 18:43 ` Peter Maydell 2020-06-08 12:02 ` Leif Lindholm 0 siblings, 1 reply; 18+ messages in thread From: Peter Maydell @ 2020-06-04 18:43 UTC (permalink / raw) To: Leif Lindholm; +Cc: Paolo Bonzini, Andrew Jones, qemu-arm, QEMU Developers On Thu, 4 Jun 2020 at 17:26, Leif Lindholm <leif@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 <leif@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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions 2020-06-04 18:43 ` Peter Maydell @ 2020-06-08 12:02 ` Leif Lindholm 2020-06-08 12:42 ` Peter Maydell 0 siblings, 1 reply; 18+ messages in thread From: Leif Lindholm @ 2020-06-08 12:02 UTC (permalink / raw) To: Peter Maydell; +Cc: Paolo Bonzini, Andrew Jones, qemu-arm, QEMU Developers Hmm ... I managed to accidentally mark this one as read. Anyway, I spent the Friday writing the RFC I just sent out instead. On Thu, Jun 04, 2020 at 19:43:06 +0100, Peter Maydell wrote: > On Thu, 4 Jun 2020 at 17:26, Leif Lindholm <leif@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 <leif@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. Well, my take is more looking at a part of the codebase that I was not previously familiar with, seeing things that seemed redundant, but not being confident enough to dismiss them outright so having to spend time investigating (and asking silly questions on list). > > > 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. Well, I think any OS that tries to be generically bootable *has* to ignore that value, although it could be useful for informational purposes. > https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/cpus.yaml > is the official list of permitted strings, incidentally. My feeling is none of the values there are appropriate (arm,armv8 indicates ARM ltd, but not aarch64 support). I made something up for the RFC set. We could always send a patch adding some qemu, or generic, target. > > 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? Reduce the delta between internal development and upstream. This is not a suggestion NUVIA has a vast army of qemu developers ready to implement all features supported by ARMv8.whatever. But sometimes all we need is an instruction trace, and for that we need to make sure any selection sequences pick the optimal ones we do intend to implement. Best Regards, Leif ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions 2020-06-08 12:02 ` Leif Lindholm @ 2020-06-08 12:42 ` Peter Maydell 0 siblings, 0 replies; 18+ messages in thread From: Peter Maydell @ 2020-06-08 12:42 UTC (permalink / raw) To: Leif Lindholm Cc: Paolo Bonzini, Andrew Jones, qemu-arm, QEMU Developers, Rob Herring On Mon, 8 Jun 2020 at 13:02, Leif Lindholm <leif@nuviainc.com> wrote: > On Thu, Jun 04, 2020 at 19:43:06 +0100, Peter Maydell wrote: > > https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/cpus.yaml > > is the official list of permitted strings, incidentally. > > My feeling is none of the values there are appropriate (arm,armv8 > indicates ARM ltd, but not aarch64 support). I made something up for > the RFC set. We could always send a patch adding some qemu, or > generic, target. arm,armv8 is the generic "for software models" target, which sounds appropriate enough to me. Anything consuming a dtb presumably already knows whether it's in AArch64 state. If you needed to be able to determine that from the device tree then I think you'd be better off defining a property for it rather than having some kind of lookup table of "these compat string values imply 64-bit and these others do not". However, I've just noticed that when the kernel added this to the CPU binding list (apparently in passing during the conversion to yaml) in commit 672951cbd1b70a9ede6f9c6eba4ed6b726d32b03 in 2018, it documented "arm,armv8", whereas the string QEMU uses when KVM is being used is "arm,arm-v8" with a hyphen (or "arm,arm-v7" for 32-bit KVM), which is what we've used since 2013. So I guess we need to also add the with-a-hyphen version to the kernel binding documentation. thanks -- PMM ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-06-08 12:47 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-04 12:55 kvm_target, QEMU_KVM_ARM_TARGET_GENERIC_V8 questions Leif Lindholm 2020-06-04 13:10 ` Peter Maydell 2020-06-04 13:10 ` Peter Maydell 2020-06-04 13:32 ` Andrew Jones 2020-06-04 13:32 ` Andrew Jones 2020-06-04 13:37 ` Peter Maydell 2020-06-04 13:37 ` Peter Maydell 2020-06-04 15:38 ` Leif Lindholm 2020-06-04 15:38 ` Leif Lindholm 2020-06-04 15:59 ` Peter Maydell 2020-06-04 15:59 ` Peter Maydell 2020-06-04 13:18 ` Andrew Jones 2020-06-04 16:03 ` Leif Lindholm 2020-06-04 16:09 ` Peter Maydell 2020-06-04 16:26 ` Leif Lindholm 2020-06-04 18:43 ` Peter Maydell 2020-06-08 12:02 ` Leif Lindholm 2020-06-08 12:42 ` Peter Maydell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.