If -cpu <cpu>,aarch64=off is used then KVM must also be used, and it
and the host must support running the vcpu in 32-bit mode. Also, if
-cpu <cpu>,aarch64=on is used, then it doesn't matter if kvm is
enabled or not.
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
target/arm/cpu64.c | 12 ++++++------
target/arm/kvm64.c | 11 +++++++++++
target/arm/kvm_arm.h | 14 ++++++++++++++
3 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 1901997a0645..946994838d8a 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -407,13 +407,13 @@ static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
* restriction allows us to avoid fixing up functionality that assumes a
* uniform execution state like do_interrupt.
*/
- if (!kvm_enabled()) {
- error_setg(errp, "'aarch64' feature cannot be disabled "
- "unless KVM is enabled");
- return;
- }
-
if (value == false) {
+ if (!kvm_enabled() || !kvm_arm_aarch32_supported(CPU(cpu))) {
+ error_setg(errp, "'aarch64' feature cannot be disabled "
+ "unless KVM is enabled and 32-bit EL1 "
+ "is supported");
+ return;
+ }
unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
} else {
set_feature(&cpu->env, ARM_FEATURE_AARCH64);
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 22d19c9aec6f..45ccda589903 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -24,7 +24,9 @@
#include "exec/gdbstub.h"
#include "sysemu/sysemu.h"
#include "sysemu/kvm.h"
+#include "sysemu/kvm_int.h"
#include "kvm_arm.h"
+#include "hw/boards.h"
#include "internals.h"
static bool have_guest_debug;
@@ -593,6 +595,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
return true;
}
+bool kvm_arm_aarch32_supported(CPUState *cpu)
+{
+ KVMState *s = KVM_STATE(current_machine->accelerator);
+ int ret;
+
+ ret = kvm_check_extension(s, KVM_CAP_ARM_EL1_32BIT);
+ return ret > 0;
+}
+
#define ARM_CPU_ID_MPIDR 3, 0, 0, 0, 5
int kvm_arch_init_vcpu(CPUState *cs)
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 2a07333c615f..812125f805a1 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -207,6 +207,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf);
*/
void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
+/**
+ * kvm_arm_aarch32_supported:
+ * @cs: CPUState
+ *
+ * Returns true if the KVM VCPU can enable AArch32 mode and false
+ * otherwise.
+ */
+bool kvm_arm_aarch32_supported(CPUState *cs);
+
/**
* kvm_arm_get_max_vm_ipa_size - Returns the number of bits in the
* IPA address space supported by KVM
@@ -247,6 +256,11 @@ static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
cpu->host_cpu_probe_failed = true;
}
+static inline bool kvm_arm_aarch32_supported(CPUState *cs)
+{
+ return false;
+}
+
static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
{
return -ENOENT;
--
2.20.1
Hi Drew,
On 6/21/19 6:34 PM, Andrew Jones wrote:
> If -cpu <cpu>,aarch64=off is used then KVM must also be used, and it
> and the host must support running the vcpu in 32-bit mode. Also, if
> -cpu <cpu>,aarch64=on is used, then it doesn't matter if kvm is
> enabled or not.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> target/arm/cpu64.c | 12 ++++++------
> target/arm/kvm64.c | 11 +++++++++++
> target/arm/kvm_arm.h | 14 ++++++++++++++
> 3 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 1901997a0645..946994838d8a 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -407,13 +407,13 @@ static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
> * restriction allows us to avoid fixing up functionality that assumes a
> * uniform execution state like do_interrupt.
> */> - if (!kvm_enabled()) {
> - error_setg(errp, "'aarch64' feature cannot be disabled "
> - "unless KVM is enabled");
> - return;
> - }
> -
> if (value == false) {
> + if (!kvm_enabled() || !kvm_arm_aarch32_supported(CPU(cpu))) {
> + error_setg(errp, "'aarch64' feature cannot be disabled "
> + "unless KVM is enabled and 32-bit EL1 "
> + "is supported");
> + return;
> + }
> unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
> } else {
> set_feature(&cpu->env, ARM_FEATURE_AARCH64);
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 22d19c9aec6f..45ccda589903 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -24,7 +24,9 @@
> #include "exec/gdbstub.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/kvm.h"
> +#include "sysemu/kvm_int.h"
> #include "kvm_arm.h"
> +#include "hw/boards.h"
> #include "internals.h"
>
> static bool have_guest_debug;
> @@ -593,6 +595,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> return true;
> }
>
> +bool kvm_arm_aarch32_supported(CPUState *cpu)
> +{
> + KVMState *s = KVM_STATE(current_machine->accelerator);
> + int ret;
> +
> + ret = kvm_check_extension(s, KVM_CAP_ARM_EL1_32BIT);
> + return ret > 0;
nit: return kvm_check_extension() should be sufficient
> +}
> +
> #define ARM_CPU_ID_MPIDR 3, 0, 0, 0, 5
>
> int kvm_arch_init_vcpu(CPUState *cs)
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index 2a07333c615f..812125f805a1 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -207,6 +207,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf);
> */
> void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
>
> +/**
> + * kvm_arm_aarch32_supported:
> + * @cs: CPUState
use kernel-doc comment style?
> + *
> + * Returns true if the KVM VCPU can enable AArch32 mode and false
> + * otherwise.
> + */
> +bool kvm_arm_aarch32_supported(CPUState *cs);
> +
> /**
> * kvm_arm_get_max_vm_ipa_size - Returns the number of bits in the
> * IPA address space supported by KVM
> @@ -247,6 +256,11 @@ static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
> cpu->host_cpu_probe_failed = true;
> }
>
> +static inline bool kvm_arm_aarch32_supported(CPUState *cs)
> +{
> + return false;
> +}
> +
> static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
> {
> return -ENOENT;
>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
On Tue, Jun 25, 2019 at 11:35:12AM +0200, Auger Eric wrote:
> Hi Drew,
>
> On 6/21/19 6:34 PM, Andrew Jones wrote:
> > If -cpu <cpu>,aarch64=off is used then KVM must also be used, and it
> > and the host must support running the vcpu in 32-bit mode. Also, if
> > -cpu <cpu>,aarch64=on is used, then it doesn't matter if kvm is
> > enabled or not.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
>
>
> > ---
> > target/arm/cpu64.c | 12 ++++++------
> > target/arm/kvm64.c | 11 +++++++++++
> > target/arm/kvm_arm.h | 14 ++++++++++++++
> > 3 files changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index 1901997a0645..946994838d8a 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -407,13 +407,13 @@ static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
> > * restriction allows us to avoid fixing up functionality that assumes a
> > * uniform execution state like do_interrupt.
> > */> - if (!kvm_enabled()) {
> > - error_setg(errp, "'aarch64' feature cannot be disabled "
> > - "unless KVM is enabled");
> > - return;
> > - }
> > -
> > if (value == false) {
> > + if (!kvm_enabled() || !kvm_arm_aarch32_supported(CPU(cpu))) {
> > + error_setg(errp, "'aarch64' feature cannot be disabled "
> > + "unless KVM is enabled and 32-bit EL1 "
> > + "is supported");
> > + return;
> > + }
> > unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
> > } else {
> > set_feature(&cpu->env, ARM_FEATURE_AARCH64);
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index 22d19c9aec6f..45ccda589903 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -24,7 +24,9 @@
> > #include "exec/gdbstub.h"
> > #include "sysemu/sysemu.h"
> > #include "sysemu/kvm.h"
> > +#include "sysemu/kvm_int.h"
> > #include "kvm_arm.h"
> > +#include "hw/boards.h"
> > #include "internals.h"
> >
> > static bool have_guest_debug;
> > @@ -593,6 +595,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> > return true;
> > }
> >
> > +bool kvm_arm_aarch32_supported(CPUState *cpu)
> > +{
> > + KVMState *s = KVM_STATE(current_machine->accelerator);
> > + int ret;
> > +
> > + ret = kvm_check_extension(s, KVM_CAP_ARM_EL1_32BIT);
> > + return ret > 0;
> nit: return kvm_check_extension() should be sufficient
Ah yes, I forgot kvm_check_extension() already converts negative
error codes to zero. I'll fix that for v3.
> > +}
> > +
> > #define ARM_CPU_ID_MPIDR 3, 0, 0, 0, 5
> >
> > int kvm_arch_init_vcpu(CPUState *cs)
> > diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> > index 2a07333c615f..812125f805a1 100644
> > --- a/target/arm/kvm_arm.h
> > +++ b/target/arm/kvm_arm.h
> > @@ -207,6 +207,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf);
> > */
> > void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
> >
> > +/**
> > + * kvm_arm_aarch32_supported:
> > + * @cs: CPUState
> use kernel-doc comment style?
This file (kvm_arm.h) doesn't appear to have a super consistent comment
style. I see some use @var: for the parameters and some have 'Returns:
...' lines as well. I'm happy to do whatever the maintainers prefer. For
now I was just trying to mimic whatever caught my eye.
> > + *
> > + * Returns true if the KVM VCPU can enable AArch32 mode and false
> > + * otherwise.
> > + */
> > +bool kvm_arm_aarch32_supported(CPUState *cs);
> > +
> > /**
> > * kvm_arm_get_max_vm_ipa_size - Returns the number of bits in the
> > * IPA address space supported by KVM
> > @@ -247,6 +256,11 @@ static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
> > cpu->host_cpu_probe_failed = true;
> > }
> >
> > +static inline bool kvm_arm_aarch32_supported(CPUState *cs)
> > +{
> > + return false;
> > +}
> > +
> > static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
> > {
> > return -ENOENT;
> >
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>
Thanks,
drew
Hi Drew,
On 6/25/19 3:34 PM, Andrew Jones wrote:
> On Tue, Jun 25, 2019 at 11:35:12AM +0200, Auger Eric wrote:
>> Hi Drew,
>>
>> On 6/21/19 6:34 PM, Andrew Jones wrote:
>>> If -cpu <cpu>,aarch64=off is used then KVM must also be used, and it
>>> and the host must support running the vcpu in 32-bit mode. Also, if
s/and it//
>>> -cpu <cpu>,aarch64=on is used, then it doesn't matter if kvm is
>>> enabled or not.
>>>
>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>
>>
>>> ---
>>> target/arm/cpu64.c | 12 ++++++------
>>> target/arm/kvm64.c | 11 +++++++++++
>>> target/arm/kvm_arm.h | 14 ++++++++++++++
>>> 3 files changed, 31 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>>> index 1901997a0645..946994838d8a 100644
>>> --- a/target/arm/cpu64.c
>>> +++ b/target/arm/cpu64.c
>>> @@ -407,13 +407,13 @@ static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
>>> * restriction allows us to avoid fixing up functionality that assumes a
>>> * uniform execution state like do_interrupt.
>>> */> - if (!kvm_enabled()) {
>>> - error_setg(errp, "'aarch64' feature cannot be disabled "
>>> - "unless KVM is enabled");
>>> - return;
>>> - }
>>> -
>>> if (value == false) {
>>> + if (!kvm_enabled() || !kvm_arm_aarch32_supported(CPU(cpu))) {
>>> + error_setg(errp, "'aarch64' feature cannot be disabled "
>>> + "unless KVM is enabled and 32-bit EL1 "
>>> + "is supported");
>>> + return;
>>> + }
>>> unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
>>> } else {
>>> set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>>> index 22d19c9aec6f..45ccda589903 100644
>>> --- a/target/arm/kvm64.c
>>> +++ b/target/arm/kvm64.c
>>> @@ -24,7 +24,9 @@
>>> #include "exec/gdbstub.h"
>>> #include "sysemu/sysemu.h"
>>> #include "sysemu/kvm.h"
>>> +#include "sysemu/kvm_int.h"
>>> #include "kvm_arm.h"
>>> +#include "hw/boards.h"
By the way those two new headers are not needed by this patch
>>> #include "internals.h"
>>>
>>> static bool have_guest_debug;
>>> @@ -593,6 +595,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>>> return true;
>>> }
>>>
>>> +bool kvm_arm_aarch32_supported(CPUState *cpu)
>>> +{
>>> + KVMState *s = KVM_STATE(current_machine->accelerator);
>>> + int ret;
>>> +
>>> + ret = kvm_check_extension(s, KVM_CAP_ARM_EL1_32BIT);
>>> + return ret > 0;
>> nit: return kvm_check_extension() should be sufficient
>
> Ah yes, I forgot kvm_check_extension() already converts negative
> error codes to zero. I'll fix that for v3.
>
>>> +}
>>> +
>>> #define ARM_CPU_ID_MPIDR 3, 0, 0, 0, 5
>>>
>>> int kvm_arch_init_vcpu(CPUState *cs)
>>> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
>>> index 2a07333c615f..812125f805a1 100644
>>> --- a/target/arm/kvm_arm.h
>>> +++ b/target/arm/kvm_arm.h
>>> @@ -207,6 +207,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf);
>>> */
>>> void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
>>>
>>> +/**
>>> + * kvm_arm_aarch32_supported:
>>> + * @cs: CPUState
>> use kernel-doc comment style?
>
> This file (kvm_arm.h) doesn't appear to have a super consistent comment
> style. I see some use @var: for the parameters and some have 'Returns:
> ...' lines as well. I'm happy to do whatever the maintainers prefer. For
> now I was just trying to mimic whatever caught my eye.>
>>> + *
>>> + * Returns true if the KVM VCPU can enable AArch32 mode and false
>>> + * otherwise.
>>> + */
>>> +bool kvm_arm_aarch32_supported(CPUState *cs);
>>> +
>>> /**
>>> * kvm_arm_get_max_vm_ipa_size - Returns the number of bits in the
>>> * IPA address space supported by KVM
>>> @@ -247,6 +256,11 @@ static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
>>> cpu->host_cpu_probe_failed = true;
>>> }
>>>
>>> +static inline bool kvm_arm_aarch32_supported(CPUState *cs)
>>> +{
>>> + return false;
>>> +}
>>> +
>>> static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
>>> {
>>> return -ENOENT;
>>>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
>>
>
> Thanks,
> drew
>
On Wed, Jul 24, 2019 at 02:51:15PM +0200, Auger Eric wrote:
> Hi Drew,
>
> On 6/25/19 3:34 PM, Andrew Jones wrote:
> > On Tue, Jun 25, 2019 at 11:35:12AM +0200, Auger Eric wrote:
> >> Hi Drew,
> >>
> >> On 6/21/19 6:34 PM, Andrew Jones wrote:
> >>> If -cpu <cpu>,aarch64=off is used then KVM must also be used, and it
> >>> and the host must support running the vcpu in 32-bit mode. Also, if
> s/and it//
"and it and the host" means "and KVM and the host", as 'it' refers to the
last subject, which is KVM. I wanted to point out both the host (machine)
and KVM (version of kernel with KVM) need to support the feature.
> >>> -cpu <cpu>,aarch64=on is used, then it doesn't matter if kvm is
> >>> enabled or not.
> >>>
> >>> Signed-off-by: Andrew Jones <drjones@redhat.com>
> >>
> >>
> >>> ---
> >>> target/arm/cpu64.c | 12 ++++++------
> >>> target/arm/kvm64.c | 11 +++++++++++
> >>> target/arm/kvm_arm.h | 14 ++++++++++++++
> >>> 3 files changed, 31 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> >>> index 1901997a0645..946994838d8a 100644
> >>> --- a/target/arm/cpu64.c
> >>> +++ b/target/arm/cpu64.c
> >>> @@ -407,13 +407,13 @@ static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
> >>> * restriction allows us to avoid fixing up functionality that assumes a
> >>> * uniform execution state like do_interrupt.
> >>> */> - if (!kvm_enabled()) {
> >>> - error_setg(errp, "'aarch64' feature cannot be disabled "
> >>> - "unless KVM is enabled");
> >>> - return;
> >>> - }
> >>> -
> >>> if (value == false) {
> >>> + if (!kvm_enabled() || !kvm_arm_aarch32_supported(CPU(cpu))) {
> >>> + error_setg(errp, "'aarch64' feature cannot be disabled "
> >>> + "unless KVM is enabled and 32-bit EL1 "
> >>> + "is supported");
> >>> + return;
> >>> + }
> >>> unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
> >>> } else {
> >>> set_feature(&cpu->env, ARM_FEATURE_AARCH64);
> >>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> >>> index 22d19c9aec6f..45ccda589903 100644
> >>> --- a/target/arm/kvm64.c
> >>> +++ b/target/arm/kvm64.c
> >>> @@ -24,7 +24,9 @@
> >>> #include "exec/gdbstub.h"
> >>> #include "sysemu/sysemu.h"
> >>> #include "sysemu/kvm.h"
> >>> +#include "sysemu/kvm_int.h"
> >>> #include "kvm_arm.h"
> >>> +#include "hw/boards.h"
> By the way those two new headers are not needed by this patch
Really?
current_machine is defined in hw/boards.h and KVM_STATE is defined
in sysemu/kvm_int.h.
> >>> #include "internals.h"
> >>>
> >>> static bool have_guest_debug;
> >>> @@ -593,6 +595,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> >>> return true;
> >>> }
> >>>
> >>> +bool kvm_arm_aarch32_supported(CPUState *cpu)
> >>> +{
> >>> + KVMState *s = KVM_STATE(current_machine->accelerator);
> >>> + int ret;
> >>> +
> >>> + ret = kvm_check_extension(s, KVM_CAP_ARM_EL1_32BIT);
> >>> + return ret > 0;
> >> nit: return kvm_check_extension() should be sufficient
> >
> > Ah yes, I forgot kvm_check_extension() already converts negative
> > error codes to zero. I'll fix that for v3.
> >
> >>> +}
> >>> +
> >>> #define ARM_CPU_ID_MPIDR 3, 0, 0, 0, 5
> >>>
> >>> int kvm_arch_init_vcpu(CPUState *cs)
> >>> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> >>> index 2a07333c615f..812125f805a1 100644
> >>> --- a/target/arm/kvm_arm.h
> >>> +++ b/target/arm/kvm_arm.h
> >>> @@ -207,6 +207,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf);
> >>> */
> >>> void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
> >>>
> >>> +/**
> >>> + * kvm_arm_aarch32_supported:
> >>> + * @cs: CPUState
> >> use kernel-doc comment style?
> >
> > This file (kvm_arm.h) doesn't appear to have a super consistent comment
> > style. I see some use @var: for the parameters and some have 'Returns:
> > ...' lines as well. I'm happy to do whatever the maintainers prefer. For
> > now I was just trying to mimic whatever caught my eye.>
> >>> + *
> >>> + * Returns true if the KVM VCPU can enable AArch32 mode and false
> >>> + * otherwise.
> >>> + */
> >>> +bool kvm_arm_aarch32_supported(CPUState *cs);
> >>> +
> >>> /**
> >>> * kvm_arm_get_max_vm_ipa_size - Returns the number of bits in the
> >>> * IPA address space supported by KVM
> >>> @@ -247,6 +256,11 @@ static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
> >>> cpu->host_cpu_probe_failed = true;
> >>> }
> >>>
> >>> +static inline bool kvm_arm_aarch32_supported(CPUState *cs)
> >>> +{
> >>> + return false;
> >>> +}
> >>> +
> >>> static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
> >>> {
> >>> return -ENOENT;
> >>>
> >> Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks,
drew
Hi Drew,
On 7/24/19 3:52 PM, Andrew Jones wrote:
> On Wed, Jul 24, 2019 at 02:51:15PM +0200, Auger Eric wrote:
>> Hi Drew,
>>
>> On 6/25/19 3:34 PM, Andrew Jones wrote:
>>> On Tue, Jun 25, 2019 at 11:35:12AM +0200, Auger Eric wrote:
>>>> Hi Drew,
>>>>
>>>> On 6/21/19 6:34 PM, Andrew Jones wrote:
>>>>> If -cpu <cpu>,aarch64=off is used then KVM must also be used, and it
>>>>> and the host must support running the vcpu in 32-bit mode. Also, if
>> s/and it//
>
> "and it and the host" means "and KVM and the host", as 'it' refers to the
> last subject, which is KVM. I wanted to point out both the host (machine)
> and KVM (version of kernel with KVM) need to support the feature.
hum ok
>
>>>>> -cpu <cpu>,aarch64=on is used, then it doesn't matter if kvm is
>>>>> enabled or not.
>>>>>
>>>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>>>
>>>>
>>>>> ---
>>>>> target/arm/cpu64.c | 12 ++++++------
>>>>> target/arm/kvm64.c | 11 +++++++++++
>>>>> target/arm/kvm_arm.h | 14 ++++++++++++++
>>>>> 3 files changed, 31 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>>>>> index 1901997a0645..946994838d8a 100644
>>>>> --- a/target/arm/cpu64.c
>>>>> +++ b/target/arm/cpu64.c
>>>>> @@ -407,13 +407,13 @@ static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
>>>>> * restriction allows us to avoid fixing up functionality that assumes a
>>>>> * uniform execution state like do_interrupt.
>>>>> */> - if (!kvm_enabled()) {
>>>>> - error_setg(errp, "'aarch64' feature cannot be disabled "
>>>>> - "unless KVM is enabled");
>>>>> - return;
>>>>> - }
>>>>> -
>>>>> if (value == false) {
>>>>> + if (!kvm_enabled() || !kvm_arm_aarch32_supported(CPU(cpu))) {
>>>>> + error_setg(errp, "'aarch64' feature cannot be disabled "
>>>>> + "unless KVM is enabled and 32-bit EL1 "
>>>>> + "is supported");
>>>>> + return;
>>>>> + }
>>>>> unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
>>>>> } else {
>>>>> set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>>>>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>>>>> index 22d19c9aec6f..45ccda589903 100644
>>>>> --- a/target/arm/kvm64.c
>>>>> +++ b/target/arm/kvm64.c
>>>>> @@ -24,7 +24,9 @@
>>>>> #include "exec/gdbstub.h"
>>>>> #include "sysemu/sysemu.h"
>>>>> #include "sysemu/kvm.h"
>>>>> +#include "sysemu/kvm_int.h"
>>>>> #include "kvm_arm.h"
>>>>> +#include "hw/boards.h"
>> By the way those two new headers are not needed by this patch
>
> Really?
>
> current_machine is defined in hw/boards.h and KVM_STATE is defined
> in sysemu/kvm_int.h.
argh my bad.
Sorry for the noise
Eric
>
>>>>> #include "internals.h"
>>>>>
>>>>> static bool have_guest_debug;
>>>>> @@ -593,6 +595,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>>>>> return true;
>>>>> }
>>>>>
>>>>> +bool kvm_arm_aarch32_supported(CPUState *cpu)
>>>>> +{
>>>>> + KVMState *s = KVM_STATE(current_machine->accelerator);
>>>>> + int ret;
>>>>> +
>>>>> + ret = kvm_check_extension(s, KVM_CAP_ARM_EL1_32BIT);
>>>>> + return ret > 0;
>>>> nit: return kvm_check_extension() should be sufficient
>>>
>>> Ah yes, I forgot kvm_check_extension() already converts negative
>>> error codes to zero. I'll fix that for v3.
>>>
>>>>> +}
>>>>> +
>>>>> #define ARM_CPU_ID_MPIDR 3, 0, 0, 0, 5
>>>>>
>>>>> int kvm_arch_init_vcpu(CPUState *cs)
>>>>> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
>>>>> index 2a07333c615f..812125f805a1 100644
>>>>> --- a/target/arm/kvm_arm.h
>>>>> +++ b/target/arm/kvm_arm.h
>>>>> @@ -207,6 +207,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf);
>>>>> */
>>>>> void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
>>>>>
>>>>> +/**
>>>>> + * kvm_arm_aarch32_supported:
>>>>> + * @cs: CPUState
>>>> use kernel-doc comment style?
>>>
>>> This file (kvm_arm.h) doesn't appear to have a super consistent comment
>>> style. I see some use @var: for the parameters and some have 'Returns:
>>> ...' lines as well. I'm happy to do whatever the maintainers prefer. For
>>> now I was just trying to mimic whatever caught my eye.>
>>>>> + *
>>>>> + * Returns true if the KVM VCPU can enable AArch32 mode and false
>>>>> + * otherwise.
>>>>> + */
>>>>> +bool kvm_arm_aarch32_supported(CPUState *cs);
>>>>> +
>>>>> /**
>>>>> * kvm_arm_get_max_vm_ipa_size - Returns the number of bits in the
>>>>> * IPA address space supported by KVM
>>>>> @@ -247,6 +256,11 @@ static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
>>>>> cpu->host_cpu_probe_failed = true;
>>>>> }
>>>>>
>>>>> +static inline bool kvm_arm_aarch32_supported(CPUState *cs)
>>>>> +{
>>>>> + return false;
>>>>> +}
>>>>> +
>>>>> static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
>>>>> {
>>>>> return -ENOENT;
>>>>>
>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>
> Thanks,
> drew
>
© 2016 - 2026 Red Hat, Inc.