| Message ID | 1521232280-13089-16-git-send-email-alindsay@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/16/2018 09:31 PM, Aaron Lindsay wrote: > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > target/arm/cpu.c | 3 +++ > target/arm/cpu.h | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index b0d032c..e544f1d 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -765,6 +765,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > /* Some features automatically imply others: */ > if (arm_feature(env, ARM_FEATURE_V8)) { > set_feature(env, ARM_FEATURE_V7); > + set_feature(env, ARM_FEATURE_V7VE); > set_feature(env, ARM_FEATURE_ARM_DIV); > set_feature(env, ARM_FEATURE_LPAE); > } > @@ -1481,6 +1482,7 @@ static void cortex_a7_initfn(Object *obj) > > cpu->dtb_compatible = "arm,cortex-a7"; > set_feature(&cpu->env, ARM_FEATURE_V7); > + set_feature(&cpu->env, ARM_FEATURE_V7VE); > set_feature(&cpu->env, ARM_FEATURE_VFP4); > set_feature(&cpu->env, ARM_FEATURE_NEON); > set_feature(&cpu->env, ARM_FEATURE_THUMB2EE); > @@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj) > > cpu->dtb_compatible = "arm,cortex-a15"; > set_feature(&cpu->env, ARM_FEATURE_V7); > + set_feature(&cpu->env, ARM_FEATURE_V7VE); > set_feature(&cpu->env, ARM_FEATURE_VFP4); > set_feature(&cpu->env, ARM_FEATURE_NEON); > set_feature(&cpu->env, ARM_FEATURE_THUMB2EE); > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index fb2f983..cc1e2fb 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -1439,6 +1439,7 @@ enum arm_features { > ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling. */ > ARM_FEATURE_THUMB2EE, > ARM_FEATURE_V7MP, /* v7 Multiprocessing Extensions */ > + ARM_FEATURE_V7VE, /* v7 with Virtualization Extensions */ > ARM_FEATURE_V4T, > ARM_FEATURE_V5, > ARM_FEATURE_STRONGARM, >
On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote: > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> > --- > target/arm/cpu.c | 3 +++ > target/arm/cpu.h | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index b0d032c..e544f1d 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -765,6 +765,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > /* Some features automatically imply others: */ > if (arm_feature(env, ARM_FEATURE_V8)) { > set_feature(env, ARM_FEATURE_V7); > + set_feature(env, ARM_FEATURE_V7VE); > set_feature(env, ARM_FEATURE_ARM_DIV); > set_feature(env, ARM_FEATURE_LPAE); > } > @@ -1481,6 +1482,7 @@ static void cortex_a7_initfn(Object *obj) > > cpu->dtb_compatible = "arm,cortex-a7"; > set_feature(&cpu->env, ARM_FEATURE_V7); > + set_feature(&cpu->env, ARM_FEATURE_V7VE); > set_feature(&cpu->env, ARM_FEATURE_VFP4); > set_feature(&cpu->env, ARM_FEATURE_NEON); > set_feature(&cpu->env, ARM_FEATURE_THUMB2EE); > @@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj) > > cpu->dtb_compatible = "arm,cortex-a15"; > set_feature(&cpu->env, ARM_FEATURE_V7); > + set_feature(&cpu->env, ARM_FEATURE_V7VE); > set_feature(&cpu->env, ARM_FEATURE_VFP4); > set_feature(&cpu->env, ARM_FEATURE_NEON); > set_feature(&cpu->env, ARM_FEATURE_THUMB2EE); > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index fb2f983..cc1e2fb 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -1439,6 +1439,7 @@ enum arm_features { > ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling. */ > ARM_FEATURE_THUMB2EE, > ARM_FEATURE_V7MP, /* v7 Multiprocessing Extensions */ > + ARM_FEATURE_V7VE, /* v7 with Virtualization Extensions */ > ARM_FEATURE_V4T, > ARM_FEATURE_V5, > ARM_FEATURE_STRONGARM, What's the difference between this and ARM_FEATURE_EL2 ? thanks -- PMM
On Apr 12 18:17, Peter Maydell wrote: > On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote: > > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> > > --- > > target/arm/cpu.c | 3 +++ > > target/arm/cpu.h | 1 + > > 2 files changed, 4 insertions(+) > > > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > > index b0d032c..e544f1d 100644 > > --- a/target/arm/cpu.c > > +++ b/target/arm/cpu.c > > @@ -765,6 +765,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > > /* Some features automatically imply others: */ > > if (arm_feature(env, ARM_FEATURE_V8)) { > > set_feature(env, ARM_FEATURE_V7); > > + set_feature(env, ARM_FEATURE_V7VE); > > set_feature(env, ARM_FEATURE_ARM_DIV); > > set_feature(env, ARM_FEATURE_LPAE); > > } > > @@ -1481,6 +1482,7 @@ static void cortex_a7_initfn(Object *obj) > > > > cpu->dtb_compatible = "arm,cortex-a7"; > > set_feature(&cpu->env, ARM_FEATURE_V7); > > + set_feature(&cpu->env, ARM_FEATURE_V7VE); > > set_feature(&cpu->env, ARM_FEATURE_VFP4); > > set_feature(&cpu->env, ARM_FEATURE_NEON); > > set_feature(&cpu->env, ARM_FEATURE_THUMB2EE); > > @@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj) > > > > cpu->dtb_compatible = "arm,cortex-a15"; > > set_feature(&cpu->env, ARM_FEATURE_V7); > > + set_feature(&cpu->env, ARM_FEATURE_V7VE); > > set_feature(&cpu->env, ARM_FEATURE_VFP4); > > set_feature(&cpu->env, ARM_FEATURE_NEON); > > set_feature(&cpu->env, ARM_FEATURE_THUMB2EE); > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > > index fb2f983..cc1e2fb 100644 > > --- a/target/arm/cpu.h > > +++ b/target/arm/cpu.h > > @@ -1439,6 +1439,7 @@ enum arm_features { > > ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling. */ > > ARM_FEATURE_THUMB2EE, > > ARM_FEATURE_V7MP, /* v7 Multiprocessing Extensions */ > > + ARM_FEATURE_V7VE, /* v7 with Virtualization Extensions */ > > ARM_FEATURE_V4T, > > ARM_FEATURE_V5, > > ARM_FEATURE_STRONGARM, > > What's the difference between this and ARM_FEATURE_EL2 ? I use ARM_FEATURE_V7VE in a later patch to guard against implementing PMOVSSET on v7 machines which don't implement the virtualization extensions (http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04917.html). I could use ARM_FEATURE_EL2, but declaring that v7 machines supported EL2 didn't feel right. I don't feel strongly one way or the other - how do you prefer to handle this? -Aaron
On 17 April 2018 at 15:23, Aaron Lindsay <alindsay@codeaurora.org> wrote: > On Apr 12 18:17, Peter Maydell wrote: >> What's the difference between this and ARM_FEATURE_EL2 ? > > I use ARM_FEATURE_V7VE in a later patch to guard against implementing > PMOVSSET on v7 machines which don't implement the virtualization > extensions > (http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04917.html). > I could use ARM_FEATURE_EL2, but declaring that v7 machines supported > EL2 didn't feel right. I don't feel strongly one way or the other - how > do you prefer to handle this? So, the underlying issue here is that there's a QEMU specific fudge going on. Architecturally, if the CPU implements the Virtualization Extensions, then: * it has Hyp mode * it must also implement the Security Extensions * on reset it starts in the Secure world * it has LPAE * it has some stuff that is not inherently tied to having EL2, like the SDIV and UDIV instructions, and the presence of PMOVSSET In an ideal world, we'd just have a feature flag that turned all that on. Unfortunately, a combination of backwards compatibility issues, the order in which various features were implemented in QEMU, and the fact that KVM can't emulate a guest CPU with the Security Extensions means that we want to be able to model variants of some CPUs that don't really exist in real hardware: Cortex-A15 and -A7 which only implement EL0/EL1 but still have all the v7VE features that you can see from those ELs. But we didn't really properly lay out guidelines for how the feature bits should work in this case, with the result that we have a bunch of local hacks (for instance get_S1prot() has a check on the LPAE feature bit, since in practice that bit is set in exactly the CPUs that have v7VE; and the UDIV/SDIV insns have their own feature bits.) So we should probably sort out this mess first, either by: (a) state that we use ARM_FEATURE_LPAE for all checks for features that are architecturally v7VE but which we want to exist even on our v7VE-no-Hyp-no-Secure oddballs (b) define an ARM_FEATURE_V7VE for them (c) define separate feature bits for them individually In any case we'd retain ARM_FEATURE_EL2 for "and really has EL2/Hyp mode", and we'd want to do an audit of current uses of various feature bits to see whether they followed the new rules. (For AArch64 things are a bit less awkward because the architecture allows the idea of an implementation that has EL2 but not EL3.) thanks -- PMM
On Apr 17 16:00, Peter Maydell wrote: > On 17 April 2018 at 15:23, Aaron Lindsay <alindsay@codeaurora.org> wrote: > > On Apr 12 18:17, Peter Maydell wrote: > >> What's the difference between this and ARM_FEATURE_EL2 ? > > > > I use ARM_FEATURE_V7VE in a later patch to guard against implementing > > PMOVSSET on v7 machines which don't implement the virtualization > > extensions > > (http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04917.html). > > I could use ARM_FEATURE_EL2, but declaring that v7 machines supported > > EL2 didn't feel right. I don't feel strongly one way or the other - how > > do you prefer to handle this? > > So, the underlying issue here is that there's a QEMU specific > fudge going on. Architecturally, if the CPU implements the > Virtualization Extensions, then: > * it has Hyp mode > * it must also implement the Security Extensions > * on reset it starts in the Secure world > * it has LPAE > * it has some stuff that is not inherently tied to having EL2, > like the SDIV and UDIV instructions, and the presence of > PMOVSSET > > In an ideal world, we'd just have a feature flag that turned > all that on. Unfortunately, a combination of backwards compatibility > issues, the order in which various features were implemented > in QEMU, and the fact that KVM can't emulate a guest CPU with > the Security Extensions means that we want to be able to model > variants of some CPUs that don't really exist in real hardware: > Cortex-A15 and -A7 which only implement EL0/EL1 but still have > all the v7VE features that you can see from those ELs. But we > didn't really properly lay out guidelines for how the feature > bits should work in this case, with the result that we have > a bunch of local hacks (for instance get_S1prot() has a check > on the LPAE feature bit, since in practice that bit is set in > exactly the CPUs that have v7VE; and the UDIV/SDIV insns have > their own feature bits.) > > So we should probably sort out this mess first, either by: > > (a) state that we use ARM_FEATURE_LPAE for all checks for > features that are architecturally v7VE but which we want to > exist even on our v7VE-no-Hyp-no-Secure oddballs Are you implying that this would only involve updating the comments to match the existing implementation? > (b) define an ARM_FEATURE_V7VE for them This seems the most attractive to me, though perhaps that's partially aspirational - it is closest to what would be happening in an ideal world where QEMU fully supported V7VE for this hardware. > (c) define separate feature bits for them individually I'm hesitant to formulate a patch for this - I'm not confident I understand the context and interactions of the pieces involved well enough to contribute productively. That said, I understand your motivation to fix this the right way and am willing to try with a little more direction/hand-holding. For instance, which CPUs are we talking about here - I only see both ARM_FEATURE_ARM_DIV and ARM_FEATURE_LPAE advertised together (without full V8) in cortex_a7_initfn and cortex_a15_initfn. I'm assuming that V8 implies the full set of V7VE features we're discussing here so whatever we come up with as a solution would be automatically set in the ARM_FEATURE_V8 "Some features automatically imply others:" `if` statement in arm_cpu_realizefn. -Aaron > In any case we'd retain ARM_FEATURE_EL2 for "and really > has EL2/Hyp mode", and we'd want to do an audit of current > uses of various feature bits to see whether they followed > the new rules. > > (For AArch64 things are a bit less awkward because the > architecture allows the idea of an implementation that > has EL2 but not EL3.)
On Apr 17 16:00, Peter Maydell wrote: > On 17 April 2018 at 15:23, Aaron Lindsay <alindsay@codeaurora.org> wrote: > > On Apr 12 18:17, Peter Maydell wrote: > >> What's the difference between this and ARM_FEATURE_EL2 ? > > > > I use ARM_FEATURE_V7VE in a later patch to guard against implementing > > PMOVSSET on v7 machines which don't implement the virtualization > > extensions > > (http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04917.html). > > I could use ARM_FEATURE_EL2, but declaring that v7 machines supported > > EL2 didn't feel right. I don't feel strongly one way or the other - how > > do you prefer to handle this? > > So, the underlying issue here is that there's a QEMU specific > fudge going on. Architecturally, if the CPU implements the > Virtualization Extensions, then: > * it has Hyp mode > * it must also implement the Security Extensions > * on reset it starts in the Secure world > * it has LPAE > * it has some stuff that is not inherently tied to having EL2, > like the SDIV and UDIV instructions, and the presence of > PMOVSSET > > In an ideal world, we'd just have a feature flag that turned > all that on. Unfortunately, a combination of backwards compatibility > issues, the order in which various features were implemented > in QEMU, and the fact that KVM can't emulate a guest CPU with > the Security Extensions means that we want to be able to model > variants of some CPUs that don't really exist in real hardware: > Cortex-A15 and -A7 which only implement EL0/EL1 but still have > all the v7VE features that you can see from those ELs. But we > didn't really properly lay out guidelines for how the feature > bits should work in this case, with the result that we have > a bunch of local hacks (for instance get_S1prot() has a check > on the LPAE feature bit, since in practice that bit is set in > exactly the CPUs that have v7VE; and the UDIV/SDIV insns have > their own feature bits.) > > So we should probably sort out this mess first, either by: > > (a) state that we use ARM_FEATURE_LPAE for all checks for > features that are architecturally v7VE but which we want to > exist even on our v7VE-no-Hyp-no-Secure oddballs > (b) define an ARM_FEATURE_V7VE for them > (c) define separate feature bits for them individually From what I can tell, using ARM_FEATURE_LPAE to represent all the almost-v7ve misfits won't work well because ARM_FEATURE_ARM_DIV may be supported on some platforms for which ARM_FEATURE_LPAE is not (Cortex R5), and ARM_FEATURE_ARM_DIV is read from ID_ISAR0 in kvm_arm_get_host_cpu_features() (and may be set/not set independently of ARM_FEATURE_LPAE). It appears there is a need to independently distinguish between them. The same reasoning also seems to rule out option (b) "one ARM_FEATURE_V7VE to rule them all", leaving me with option (c). It almost seems silly to create ARM_FEATURE_PMOVSSET, but I'm not sure what else makes sense to do here. Am I missing something (I'm almost hoping I am)? -Aaron
On 17 May 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote: > On Apr 17 16:00, Peter Maydell wrote: >> So, the underlying issue here is that there's a QEMU specific >> fudge going on. Architecturally, if the CPU implements the >> Virtualization Extensions, then: >> * it has Hyp mode >> * it must also implement the Security Extensions >> * on reset it starts in the Secure world >> * it has LPAE >> * it has some stuff that is not inherently tied to having EL2, >> like the SDIV and UDIV instructions, and the presence of >> PMOVSSET >> >> In an ideal world, we'd just have a feature flag that turned >> all that on. Unfortunately, a combination of backwards compatibility >> issues, the order in which various features were implemented >> in QEMU, and the fact that KVM can't emulate a guest CPU with >> the Security Extensions means that we want to be able to model >> variants of some CPUs that don't really exist in real hardware: >> Cortex-A15 and -A7 which only implement EL0/EL1 but still have >> all the v7VE features that you can see from those ELs. But we >> didn't really properly lay out guidelines for how the feature >> bits should work in this case, with the result that we have >> a bunch of local hacks (for instance get_S1prot() has a check >> on the LPAE feature bit, since in practice that bit is set in >> exactly the CPUs that have v7VE; and the UDIV/SDIV insns have >> their own feature bits.) >> >> So we should probably sort out this mess first, either by: >> >> (a) state that we use ARM_FEATURE_LPAE for all checks for >> features that are architecturally v7VE but which we want to >> exist even on our v7VE-no-Hyp-no-Secure oddballs >> (b) define an ARM_FEATURE_V7VE for them >> (c) define separate feature bits for them individually > > From what I can tell, using ARM_FEATURE_LPAE to represent all the > almost-v7ve misfits won't work well because ARM_FEATURE_ARM_DIV may be > supported on some platforms for which ARM_FEATURE_LPAE is not (Cortex > R5), and ARM_FEATURE_ARM_DIV is read from ID_ISAR0 in > kvm_arm_get_host_cpu_features() (and may be set/not set independently of > ARM_FEATURE_LPAE). It appears there is a need to independently > distinguish between them. We need (and already have) a separate feature bit for ARM_DIV exactly because it's present on some CPUs which don't have V7VE; so we don't want to roll that back into a V7VE feature bit. > The same reasoning also seems to rule out > option (b) "one ARM_FEATURE_V7VE to rule them all", leaving me with > option (c). > > It almost seems silly to create ARM_FEATURE_PMOVSSET, but I'm not sure > what else makes sense to do here. Am I missing something (I'm almost > hoping I am)? Sorry, I forgot I hadn't replied to this email yet. Let's do this: * define a new ARM_FEATURE_V7VE: ARM_FEATURE_V7VE, /* v7 Virtualization Extensions (non-EL2 parts) */ In arm_cpu_realizefn: * change the existing "FEATURE_V8 implies V7/ARM_DIV/LPAE" to "FEATURE_V8 implies V7VE" * below that put if (arm_feature(env, ARM_FEATURE_V7VE) { /* v7 Virtualization Extensions. In real hardware this implies * EL2 and also the presence of the Security Extensions. * For QEMU, for backwards-compatibility we implement some * CPUs or CPU configs which have no actual EL2 or EL3 but do * include the various other features that V7VE implies. * Presence of EL2 itself is ARM_FEATURE_EL2, and of the * Security Extensions is ARM_FEATURE_EL3. */ set_feature(env, ARM_FEATURE_ARM_DIV); set_feature(env, ARM_FEATURE_LPAE); set_feature(env, ARM_FEATURE_V7); } * in kvm_arm_get_host_cpu_features() in kvm32.c add set_feature(&features, ARM_FEATURE_V7VE); where we currently set V7, LPAE, etc. (by definition a host CPU which supports KVM has v7VE.) * perhaps some followon patches that correct checks that were testing something else and can now use the new V7VE feature bit. thanks -- PMM
On May 31 15:18, Peter Maydell wrote: > On 17 May 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote: > > On Apr 17 16:00, Peter Maydell wrote: > >> So, the underlying issue here is that there's a QEMU specific > >> fudge going on. Architecturally, if the CPU implements the > >> Virtualization Extensions, then: > >> * it has Hyp mode > >> * it must also implement the Security Extensions > >> * on reset it starts in the Secure world > >> * it has LPAE > >> * it has some stuff that is not inherently tied to having EL2, > >> like the SDIV and UDIV instructions, and the presence of > >> PMOVSSET > >> > >> In an ideal world, we'd just have a feature flag that turned > >> all that on. Unfortunately, a combination of backwards compatibility > >> issues, the order in which various features were implemented > >> in QEMU, and the fact that KVM can't emulate a guest CPU with > >> the Security Extensions means that we want to be able to model > >> variants of some CPUs that don't really exist in real hardware: > >> Cortex-A15 and -A7 which only implement EL0/EL1 but still have > >> all the v7VE features that you can see from those ELs. But we > >> didn't really properly lay out guidelines for how the feature > >> bits should work in this case, with the result that we have > >> a bunch of local hacks (for instance get_S1prot() has a check > >> on the LPAE feature bit, since in practice that bit is set in > >> exactly the CPUs that have v7VE; and the UDIV/SDIV insns have > >> their own feature bits.) > >> > >> So we should probably sort out this mess first, either by: > >> > >> (a) state that we use ARM_FEATURE_LPAE for all checks for > >> features that are architecturally v7VE but which we want to > >> exist even on our v7VE-no-Hyp-no-Secure oddballs > >> (b) define an ARM_FEATURE_V7VE for them > >> (c) define separate feature bits for them individually > > > > From what I can tell, using ARM_FEATURE_LPAE to represent all the > > almost-v7ve misfits won't work well because ARM_FEATURE_ARM_DIV may be > > supported on some platforms for which ARM_FEATURE_LPAE is not (Cortex > > R5), and ARM_FEATURE_ARM_DIV is read from ID_ISAR0 in > > kvm_arm_get_host_cpu_features() (and may be set/not set independently of > > ARM_FEATURE_LPAE). It appears there is a need to independently > > distinguish between them. > > We need (and already have) a separate feature bit for ARM_DIV > exactly because it's present on some CPUs which don't have V7VE; > so we don't want to roll that back into a V7VE feature bit. > > > The same reasoning also seems to rule out > > option (b) "one ARM_FEATURE_V7VE to rule them all", leaving me with > > option (c). > > > > It almost seems silly to create ARM_FEATURE_PMOVSSET, but I'm not sure > > what else makes sense to do here. Am I missing something (I'm almost > > hoping I am)? > > Sorry, I forgot I hadn't replied to this email yet. Let's do this: No problem at all - to be fair, my responses have been a bit sporadic, too. I'm working on putting your suggestions into patch form, with one clarification below. I also believe I owe you a response on another thread or two before I push out the next version of this patchset. > > * define a new ARM_FEATURE_V7VE: > > ARM_FEATURE_V7VE, /* v7 Virtualization Extensions (non-EL2 parts) */ > > In arm_cpu_realizefn: > > * change the existing "FEATURE_V8 implies V7/ARM_DIV/LPAE" to > "FEATURE_V8 implies V7VE" > * below that put > > if (arm_feature(env, ARM_FEATURE_V7VE) { > /* v7 Virtualization Extensions. In real hardware this implies > * EL2 and also the presence of the Security Extensions. > * For QEMU, for backwards-compatibility we implement some > * CPUs or CPU configs which have no actual EL2 or EL3 but do > * include the various other features that V7VE implies. > * Presence of EL2 itself is ARM_FEATURE_EL2, and of the > * Security Extensions is ARM_FEATURE_EL3. > */ > set_feature(env, ARM_FEATURE_ARM_DIV); Is it safe to assume from your comment above regarding keeping ARM_DIV separate from V7VE that the inclusion of it here is an oversight and that only LPAE and V7 should be set if V7VE is? (and that V8 should now directly imply both V7VE and ARM_DIV?) > set_feature(env, ARM_FEATURE_LPAE); > set_feature(env, ARM_FEATURE_V7); > } > > * in kvm_arm_get_host_cpu_features() in kvm32.c add > set_feature(&features, ARM_FEATURE_V7VE); > where we currently set V7, LPAE, etc. > (by definition a host CPU which supports KVM has v7VE.) Thanks! -Aaron
On 31 May 2018 at 21:39, Aaron Lindsay <alindsay@codeaurora.org> wrote: > On May 31 15:18, Peter Maydell wrote: >> if (arm_feature(env, ARM_FEATURE_V7VE) { >> /* v7 Virtualization Extensions. In real hardware this implies >> * EL2 and also the presence of the Security Extensions. >> * For QEMU, for backwards-compatibility we implement some >> * CPUs or CPU configs which have no actual EL2 or EL3 but do >> * include the various other features that V7VE implies. >> * Presence of EL2 itself is ARM_FEATURE_EL2, and of the >> * Security Extensions is ARM_FEATURE_EL3. >> */ >> set_feature(env, ARM_FEATURE_ARM_DIV); > > Is it safe to assume from your comment above regarding keeping ARM_DIV > separate from V7VE that the inclusion of it here is an oversight and > that only LPAE and V7 should be set if V7VE is? (and that V8 should > now directly imply both V7VE and ARM_DIV?) No; V7VE always implies ARM_DIV. (ARM_DIV doesn't imply V7VE, though, which is why it is a separate feature bit.) thanks -- PMM
On Jun 01 09:57, Peter Maydell wrote: > On 31 May 2018 at 21:39, Aaron Lindsay <alindsay@codeaurora.org> wrote: > > On May 31 15:18, Peter Maydell wrote: > >> if (arm_feature(env, ARM_FEATURE_V7VE) { > >> /* v7 Virtualization Extensions. In real hardware this implies > >> * EL2 and also the presence of the Security Extensions. > >> * For QEMU, for backwards-compatibility we implement some > >> * CPUs or CPU configs which have no actual EL2 or EL3 but do > >> * include the various other features that V7VE implies. > >> * Presence of EL2 itself is ARM_FEATURE_EL2, and of the > >> * Security Extensions is ARM_FEATURE_EL3. > >> */ > >> set_feature(env, ARM_FEATURE_ARM_DIV); > > > > Is it safe to assume from your comment above regarding keeping ARM_DIV > > separate from V7VE that the inclusion of it here is an oversight and > > that only LPAE and V7 should be set if V7VE is? (and that V8 should > > now directly imply both V7VE and ARM_DIV?) > > No; V7VE always implies ARM_DIV. (ARM_DIV doesn't imply V7VE, > though, which is why it is a separate feature bit.) Okay, then I'm confused about some of the preexisting logic in kvm_arm_get_host_cpu_features. The preexisting code in that function sets ARM_DIV and THUMB_DIV based on the appropriate bits in ID_ISAR0. If we already knew that > (by definition a host CPU which supports KVM has v7VE.) and that all V7VE CPUs have ARM_DIV, why did the code there bother checking ID_ISAR0 to begin with? > switch (extract32(id_isar0, 24, 4)) { > case 1: > set_feature(&features, ARM_FEATURE_THUMB_DIV); > break; > case 2: > set_feature(&features, ARM_FEATURE_ARM_DIV); > set_feature(&features, ARM_FEATURE_THUMB_DIV); > break; > default: > break; > } Should this switch/case be removed entirely? -Aaron
On 1 June 2018 at 16:34, Aaron Lindsay <alindsay@codeaurora.org> wrote: > On Jun 01 09:57, Peter Maydell wrote: >> No; V7VE always implies ARM_DIV. (ARM_DIV doesn't imply V7VE, >> though, which is why it is a separate feature bit.) > > Okay, then I'm confused about some of the preexisting logic in > kvm_arm_get_host_cpu_features. The preexisting code in that function > sets ARM_DIV and THUMB_DIV based on the appropriate bits in ID_ISAR0. If > we already knew that >> (by definition a host CPU which supports KVM has v7VE.) > and that all V7VE CPUs have ARM_DIV, why did the code there bother > checking ID_ISAR0 to begin with? Good question. I suspect I'd just forgotten that at the point when I wrote that KVM code. >> switch (extract32(id_isar0, 24, 4)) { >> case 1: >> set_feature(&features, ARM_FEATURE_THUMB_DIV); >> break; >> case 2: >> set_feature(&features, ARM_FEATURE_ARM_DIV); >> set_feature(&features, ARM_FEATURE_THUMB_DIV); >> break; >> default: >> break; >> } > > Should this switch/case be removed entirely? Yes, I think so (and also the id_isar0 variable and the idregs[] entry which arranges to initialize it). We should just set the THUMB_DIV and ARM_DIV features explicitly. thanks -- PMM
On Jun 01 16:59, Peter Maydell wrote: > On 1 June 2018 at 16:34, Aaron Lindsay <alindsay@codeaurora.org> wrote: > >> switch (extract32(id_isar0, 24, 4)) { > >> case 1: > >> set_feature(&features, ARM_FEATURE_THUMB_DIV); > >> break; > >> case 2: > >> set_feature(&features, ARM_FEATURE_ARM_DIV); > >> set_feature(&features, ARM_FEATURE_THUMB_DIV); > >> break; > >> default: > >> break; > >> } > > > > Should this switch/case be removed entirely? > > Yes, I think so (and also the id_isar0 variable and the > idregs[] entry which arranges to initialize it). We should > just set the THUMB_DIV and ARM_DIV features explicitly. Okay, thanks for straightening me out! I sensed there was some inconsistency, but misjudged exactly where. I'll include this change in my next patchset. -Aaron
diff --git a/target/arm/cpu.c b/target/arm/cpu.c index b0d032c..e544f1d 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -765,6 +765,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) /* Some features automatically imply others: */ if (arm_feature(env, ARM_FEATURE_V8)) { set_feature(env, ARM_FEATURE_V7); + set_feature(env, ARM_FEATURE_V7VE); set_feature(env, ARM_FEATURE_ARM_DIV); set_feature(env, ARM_FEATURE_LPAE); } @@ -1481,6 +1482,7 @@ static void cortex_a7_initfn(Object *obj) cpu->dtb_compatible = "arm,cortex-a7"; set_feature(&cpu->env, ARM_FEATURE_V7); + set_feature(&cpu->env, ARM_FEATURE_V7VE); set_feature(&cpu->env, ARM_FEATURE_VFP4); set_feature(&cpu->env, ARM_FEATURE_NEON); set_feature(&cpu->env, ARM_FEATURE_THUMB2EE); @@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj) cpu->dtb_compatible = "arm,cortex-a15"; set_feature(&cpu->env, ARM_FEATURE_V7); + set_feature(&cpu->env, ARM_FEATURE_V7VE); set_feature(&cpu->env, ARM_FEATURE_VFP4); set_feature(&cpu->env, ARM_FEATURE_NEON); set_feature(&cpu->env, ARM_FEATURE_THUMB2EE); diff --git a/target/arm/cpu.h b/target/arm/cpu.h index fb2f983..cc1e2fb 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1439,6 +1439,7 @@ enum arm_features { ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling. */ ARM_FEATURE_THUMB2EE, ARM_FEATURE_V7MP, /* v7 Multiprocessing Extensions */ + ARM_FEATURE_V7VE, /* v7 with Virtualization Extensions */ ARM_FEATURE_V4T, ARM_FEATURE_V5, ARM_FEATURE_STRONGARM,
Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> --- target/arm/cpu.c | 3 +++ target/arm/cpu.h | 1 + 2 files changed, 4 insertions(+)